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

OpamSystem.t_resolve_command: use ocaml code for looking up commands in PATH #4072

Merged
merged 1 commit into from
Mar 2, 2020

Conversation

Armael
Copy link
Member

@Armael Armael commented Jan 24, 2020

...instead of calling the shell (on linux).

The motivation is performance. It turns out that the speedup of #4064 is due not only to less time being spent launching processes, but also less time being spent calling to the shell in order to resolve the command. Doing the resolution (using PATH) directly in ocaml should be faster.

Note that command resolution was already done in ocaml on Windows, so this PR mostly merges the (previously) windows-specific code with the linux code.

The PR also attempts to streamline a bit the handling of Not_found vs Denied. The behavior is mostly unchanged when looking up a fully specified path (I added a Not_found case for increased precision; previously, a non-existing binary would be reported as "denied").
When a command is looked up in PATH, I follow the shell behavior a bit more consistently (as specified here "Other environment variables) and skip binaries that do not have the right permissions.
If no suitable binary is found, then Not_found is returned (the previous code was inconsistent: it would've returned Denied under windows, but Not_found under linux).

…in PATH

...instead of calling the shell (on linux).

Note that command resolution was already done in ocaml on Windows, so
this PR mostly merges the (previously) windows-specific code with the
linux code.
@Armael
Copy link
Member Author

Armael commented Jan 27, 2020

(incidentally, this PR happens to be fixing the cygwin CI (!))

@AltGr
Copy link
Member

AltGr commented Mar 2, 2020

This is great, thanks a bunch :)

@AltGr AltGr merged commit 942cd0b into ocaml:master Mar 2, 2020
@rjbou rjbou added this to the 2.1.0 milestone Mar 3, 2020
@dra27
Copy link
Member

dra27 commented Jul 8, 2020

Coming very late to this, for which apologies - this didn't fix Cygwin CI, it masked the problem, owing I think to a bug in the implementation. Beforehand, command -v cmd was correctly returning /cygdrive/c/WINDOWS/system32/cmd and the permissions check was failing, so an exception was raised. After this change, the function considers /cygdrive/c/WINDOWS/system32 in PATH but because it fails the permissions check (as it was before) it continues on to the next item in PATH. Except it never finds cmd in any other directory so now resolving the command returns `Not_found instead of `Denied.

Failing the permissions check is valid to skip on to the next directory in PATH, but it's incorrect (I think) to return `Not_found if any of the directories did contain the command, just with the appropriate permissions.

@Armael
Copy link
Member Author

Armael commented Jul 14, 2020

Ah, wow, that makes sense indeed. Sorry for introducing that trick bug!

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.

4 participants