Skip to content
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

Fix import caching #702

Merged
merged 2 commits into from
Nov 24, 2018
Merged

Fix import caching #702

merged 2 commits into from
Nov 24, 2018

Conversation

Gabriella439
Copy link
Collaborator

This fixes an apparently very old bug in import caching caught by @basile-henry

Before this change the import resolution algorithm was:

  1. Retrieving the cache
  2. Transitively resolving all imports
  3. Setting the new cache to be current import insert into the cache retrieved in
    step 1

The bug is that all of the transitive imports resolved in step 2 added
entries of their own to the cache and those new cache entries were being
clobbered by step 3.

The fix is simple: don't use the cache retrieved in step 1 to compute
the updated cache in step 3. Rather, use modify instead of put to
create the new cache so that we preserve newly-added cache entries.

This fixes an apparently very old bug in import caching caught by @basile-henry

Before this change the import resolution algorithm was:

1. Retrieving the cache
2. Transitively resolving all imports
3. Setting the new cache to be current import insert into the cache retrieved in
   step 1

The bug is that all of the transitive imports resolved in step 2 added
entries of their own to the cache and those new cache entries were being
clobbered by step 3.

The fix is simple: don't use the cache retrieved in step 1 to compute
the updated cache in step 3.  Rather, use `modify` instead of `put` to
create the new cache so that we preserve newly-added cache entries.
@Gabriella439 Gabriella439 merged commit 99fabea into master Nov 24, 2018
@Gabriella439 Gabriella439 deleted the gabriel/fix_caching_2 branch November 24, 2018 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant