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

Disable caching in OpamSystem.resolve_command #3164

Merged
merged 1 commit into from Jan 12, 2018

Conversation

Projects
None yet
3 participants
@dra27
Contributor

dra27 commented Jan 10, 2018

OpamSystem.resolve_command caches results. This cannot work - the ocamlfind package, for example, installs an ocaml script which is intended to wrap the system-installed ocaml command. This script is not invoked if ocaml has been previously executed, because the path to the system-installed ocaml command is cached.

There are various shims which could mitigate this for opam-installed binaries/scripts only, but it seems better just not to cache the results at all.

Disable caching in OpamSystem.resolve_command
Conceptually flawed, as it doesn't allow for overriding commands (for
example, opam-repository's ocamlfind package installs a custom ocaml
script when installed for a system compiler intended to wrap the actual
ocaml command)

Signed-off-by: David Allsopp <david.allsopp@metastack.com>
@AltGr

This comment has been minimized.

Member

AltGr commented Jan 11, 2018

Nicely spotted!
The cache was indeed initially planned for commands used internally (e.g. tar, patch, git, etc.) where it really should be harmless (and makes the output of strace a bit tidier). But yes, applying it to packages' command scripts is a big mistake indeed.

Could it be worth adding an optional argument to disable the cache only for commads from scripts ?

@dra27

This comment has been minimized.

Contributor

dra27 commented Jan 11, 2018

I think it's correct to view it as an unnecessary optimisation (i.e. running those commands will take vastly more time than locating them). Personally, I'd drop it completely - it's already been a subtle issue, and it feels like it could be a subtle one in future (imagine if/when ocaml-git starts installing a git command and we expect that to be used).

If opam's internal invocations want to cache the location of tar, patch, git and so forth then I think it should do that at opam init time and record the location of the tool in a config file/cache (invalidated only if the tool subsequently can't be found). i.e. it should be more like a configurable option (like MAKE=make, etc.)

@avsm

This comment has been minimized.

Member

avsm commented Jan 11, 2018

If this PR could be merged soon, it'll make a huge usability difference to me. Right now local switches are very unreliable on the Mac with a system switch due to this.

@AltGr

This comment has been minimized.

Member

AltGr commented Jan 12, 2018

Yup, merging, I'll consider re-adding for non-script commands later. Thanks!

@AltGr AltGr merged commit a540e8f into ocaml:master Jan 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dra27 dra27 deleted the dra27:remove-resolver-cache branch Apr 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment