-
-
Notifications
You must be signed in to change notification settings - Fork 320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Partial cache read fix #3834
base: master
Are you sure you want to change the base?
Conversation
…tions shutil.SameFileError, IOError for the default copy shutil.copy(). Move notice that file has been retrieved until after the file has actually been copied and didn't thrown exception
env.copy_from_cache(cachefile, t.get_internal_path()) | ||
try: | ||
env.copy_from_cache(cachefile, t.get_internal_path()) | ||
except (shutil.SameFileError, IOError) as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the SameFileError
is going to leave a remnant, as it's raised before any copying is attempted.
Upgrading to the latest version of SCons and sad to discover my local patching for this issue is still valuable. Also seems like a slight overlap with #4085? |
Wouldn't it be fun if "transient" cache problems were easier to reproduce on demand for testing? Yeah, we should revisit this. Usually when something is tagged WIP it tends not to get merged until the reason for it having been flagged Work-In-Progress is resolved. There doesn't seem to be a lot of trail here as to why this is WIP. |
# to. | ||
t.fs.unlink(t.get_internal_path()) | ||
cd.CacheDebug('CacheRetrieve(%s): Error while retrieving from %s deleting %s\n', t, cachefile) | ||
raise e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here shouldn't we behave as if the file were not in the cache, i.e. return 1, rather than raise?
Continuing the work from #3819
Contributor Checklist:
CHANGES.txt
(and read theREADME.rst
)