Prevent files from being partially read from the cache

[ravenAtSafe]

This absolutely doesn't meet the standards presented in https://scons.org/guidelines.html

e.g. I'm working with SCons 3 and Python 2.7 on Linux so no testing with Python 3 or Windows

I'm open to tweaking CHANGES.txt and README.rst but I'm not sure I can get the time to add unit tests etc. (I have done very little SCons development so this is all fairly expensive for me to figure out.)

Contributor Checklist:

• I have created a new test or updated the unit tests to cover the new/changed functionality.
• I have updated CHANGES.txt (and read the README.rst)
• I have updated the appropriate documentation

[simontegelid]

Raise what? The original exception from env.copy_from_cache or t.fs.unlink? This behaves differently in python2 and 3:

$ cat testexc.py
try:
    raise ValueError
except:
    try:
        raise AttributeError
    except:
        pass
    raise

$ python2.7 testexc.py
Traceback (most recent call last):
  File "testexc.py", line 6, in <module>
    raise AttributeError
AttributeError

$ python3.8 testexc.py
Traceback (most recent call last):
  File "testexc.py", line 3, in <module>
    raise ValueError
ValueError

I guess that this is what we're after:

$ cat testexc.py
try:
    raise ValueError
except Exception as e1:
    try:
        raise AttributeError
    except Exception as e2:
        pass
    finally:
        raise e1

$ python2.7 testexc.py
Traceback (most recent call last):
  File "testexc.py", line 10, in <module>
    raise e1
ValueError

$ python3.8 testexc.py
Traceback (most recent call last):
  File "testexc.py", line 10, in <module>
    raise e1
  File "testexc.py", line 3, in <module>
    raise ValueError
ValueError

We should probably catch only the expected exceptions, and not all of them.

[mwichmann]

We should probably catch only the expected exceptions, and not all of them.

indeed - the CI checks include linters on modified/added code, and the Sider checker is in fact complaining on these (E722 do not use bare 'except')

[mwichmann]

Hmmm, an irritation here is we don't know for sure what the underlying retrieval method will be, due to layers of abstraction. It defaults to shutil.copy2; shutil.copy if the decider is timestamp-newer, but could presumably be changed.

[ravenAtSafe]

Returning to this at last. While trying to add a unit test I found that there don't exist any unit tests for CacheRetrieveFunc, and perhaps with good reason due to the file IO?

Should I forgo a unit test or mock up enough bits and pieces to make one? (I think it's possible, at least for my throwing case)

Or I could move the fix into Taskmaster.py and test in way the existing unit tests work?

[mwichmann]

The tests aim at CacheRetrieve, which is the Action built up using CacheRetrieveFunc.

[ravenAtSafe]

I agree, but also believe every CacheRetrieve test in CacheDirTests.py mocks out CacheRetrieveFunc so it ends up having no coverage. Am I misreading? Perhaps there is a better example test in there than I realize.

[bdbaddog]

Closing.
Continuing work in PR #3834