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

OPAMVERBOSE: Fix level by flipping the env_level implementation #5686

Merged
merged 3 commits into from Sep 26, 2023

Conversation

smorimoto
Copy link
Member

@smorimoto smorimoto commented Sep 25, 2023

This literally flips the implementation. As you can see in the implementation of bool_of_string, the original implementation does not seem correct.

opam/src/core/opamStd.ml

Lines 1666 to 1670 in b0cb137

let bool_of_string s =
match String.lowercase_ascii s with
| "" | "0" | "no" | "false" -> Some false
| "1" | "yes" | "true" -> Some true
| _ -> None

In fact, I found this when debugging a GitHub Actions issue, and you can easily reproduce the issue on your machine by just try each of the following commands:

OPAMVERBOSE=no opam update
OPAMVERBOSE=yes opam update

@smorimoto
Copy link
Member Author

I was not sure where to put this PR in the changelog.

@smorimoto smorimoto marked this pull request as ready for review September 25, 2023 08:38
@rjbou rjbou self-requested a review September 25, 2023 08:41
@rjbou rjbou changed the title Fix level by flipping the env_level implementation OPAMVERBOSE: Fix level by flipping the env_level implementation Sep 25, 2023
@rjbou
Copy link
Collaborator

rjbou commented Sep 25, 2023

Well spotted! I've updated the PR with some tests & filled changes. Thanks!

@rjbou rjbou merged commit 49346c3 into ocaml:master Sep 26, 2023
29 checks passed
Opam 2.2.0 automation moved this from For alpha3 to Done Sep 26, 2023
@rjbou
Copy link
Collaborator

rjbou commented Sep 26, 2023

Thanks!

@smorimoto smorimoto deleted the fix-env-level branch September 26, 2023 18:54
@smorimoto
Copy link
Member Author

Likewise!

rjbou added a commit to rjbou/opam that referenced this pull request Oct 16, 2023
OPAMVERBOSE: Fix `level` by flipping the `env_level` implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Opam 2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants