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 esy libev detection #755

Merged
merged 2 commits into from
Dec 18, 2019
Merged

Fix esy libev detection #755

merged 2 commits into from
Dec 18, 2019

Conversation

anmonteiro
Copy link
Contributor

I'm gonna look separately into getting a test for this as suggested in #680

match result.stdout, result.exit_code with
| "true\n", 0 -> true
| _, 0 -> false
| _, _ -> raise Not_found
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you do one of these two things:

  1. Separate the test for the error code into an outer match statement, and raise Failure instead of Not_found, where the payload of Failure will explain what it is that failed, as a form of documentation in the source (even though the message will never be used at runtime).

  2. (My preference) Keep this combined match, but change the second case from _, 0 to "false\n", 0, which will make it clear that the opam command doesn't indicate that libev is not installed by returning a non-zero error code, but a non-zero error code means either a more serious failure in opam, or the opam command is missing. In this case, I'd still suggest to change the exception Not_found to Failure with a descriptive payload. I found Not_found to be misleading, because it first reads like the code means that it is specifically libev is not found by opam.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure, I'll do 2.

@aantron aantron merged commit 17e04a9 into ocsigen:master Dec 18, 2019
@aantron
Copy link
Collaborator

aantron commented Dec 18, 2019

Thank you!

@anmonteiro anmonteiro deleted the anmonteiro/esy-libev-detection branch December 18, 2019 08:30
aantron pushed a commit that referenced this pull request Dec 20, 2019
This fixes an issue where after #755 Esy would _only_ work if libev was
available in the sandbox.
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

2 participants