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

Actually permit multiple state caches to co-exist #4934

Merged
merged 1 commit into from
Dec 22, 2021

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Nov 26, 2021

Apropos this morning's dev meeting, it turns out that #4642 is as correct as @AltGr's assertion in #4930 (comment) 😀

OpamRepositoryState.Cache.save clears all the other caches away, which
is correct except when initialising the cache for the first time, which rather defeats the point. This PR ensures that when the cache is written for the first time, it doesn't erase other caches. All the other (external) paths to the repo cache go via the normal save method, and will correctly clear any other caches out of the way.

The behaviour can be seen with this repro for #4554 between opam 2.1.1 and opam-state 2.1.0:

FROM ocaml/opam
RUN sudo ln -f /usr/bin/opam-2.1 /usr/bin/opam && opam init --reinit -a
RUN cd ~/opam-repository/packages/opam-0install/opam-0install.0.4.2 ; echo 'flags: plugin' >> opam ; git add -u ; git commit -m 'flags' ; opam update
RUN opam install opam-0install opam-state.2.1.0
RUN ls -l ~/.opam/repo
RUN opam 0install dune > /dev/null
RUN ls -l ~/.opam/repo
RUN opam info dune > /dev/null
RUN ls -l ~/.opam/repo

The three ls calls will show:

  1. state-028890EA.cache (opam 2.1.1 state cache)
  2. state-392B1E43.cache (opam-state 2.1.0 state cache, the other one is deleted)
  3. state-028890EA.cache (opam 2.1.1 state cache; again the other one is deleted)

Switch in opam 2.1 + this PR, you get:

  1. state-18227757.cache (opam 2.1.2* state cache)
  2. state-392B1E43.cache (opam-state 2.1.0 state cache, the 2.1.2 cache is incorrectly deleted by opam-state 2.1.0)
  3. state-18227757.cache and state-392B1E43.cache (opam 2.1.2* regenerates its state cache and now correctly leaves the other one alone)

Then pin opam-state to this PR, and you get the complete correct outcome:

  1. state-18227757.cache (opam 2.1.2* state cache)
  2. state-3F983742.cache and state-18227757.cache (opam-state 2.1.2* correctly adds its cache)
  3. state-3F983742.cache and state-18227757.cache (no change)

OpamRepositoryState.Cache.save clears all the other caches away, which
is correct _except_ when initialising the cache for the first time.
@dra27 dra27 added this to the 2.1.3 milestone Nov 26, 2021
@rjbou rjbou linked an issue Nov 26, 2021 that may be closed by this pull request
@kit-ty-kate
Copy link
Member

Is there a way to add that to the testsuite without too much effort?

@rjbou
Copy link
Collaborator

rjbou commented Nov 26, 2021

I can try to add one, "just" have to simulate the presence of other caches

@dra27
Copy link
Member Author

dra27 commented Nov 26, 2021

Is there enough in the docker framework for depexts to test something like this - in this case, we want an "old" released version of opam (e.g. 2.1.0) testing against the PR and ensure that both state files exist at the points expected?

@rjbou rjbou modified the milestones: 2.1.3, 2.2.0~alpha Dec 1, 2021
@AltGr AltGr merged commit 44d7a7c into ocaml:master Dec 22, 2021
rjbou pushed a commit to rjbou/opam that referenced this pull request Apr 22, 2022
Actually permit multiple state caches to co-exist
@rjbou rjbou mentioned this pull request Apr 22, 2022
4 tasks
rjbou pushed a commit to rjbou/opam that referenced this pull request Apr 22, 2022
Actually permit multiple state caches to co-exist
rjbou pushed a commit to rjbou/opam that referenced this pull request Apr 27, 2022
Actually permit multiple state caches to co-exist
rjbou pushed a commit to rjbou/opam that referenced this pull request Jul 12, 2022
Actually permit multiple state caches to co-exist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[opam 2.1] opam list --installed is really slow
4 participants