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

Set OPAMCLI=2.0 by default during compilation/install/remove phases #4492

Merged
merged 4 commits into from
Jan 12, 2021

Conversation

kit-ty-kate
Copy link
Member

People who want to use the 2.1 features should be using --cli=2.1 or build-env: OPAMCLI = "2.1"

Tested successfully using the following opam files:

opam-version: "2.0"
build: ["env"]
install: ["env"]
remove: ["env"]
opam-version: "2.0"
build-env: [OPAMCLI = "2.1"]
build: ["env"]
install: ["env"]
remove: ["env"]

Fixes #4491

People who want to use the 2.1 features should be using --cli=2.1 or build-env: OPAMCLI = "2.1"
@kit-ty-kate
Copy link
Member Author

I've been trying to add tests for this, but I'm unsure where I should put it. Any idea?

@rjbou
Copy link
Collaborator

rjbou commented Jan 7, 2021

I've added a test in cli versioning one, can you check that it is exhaustive, what you had in mind to test ?
I'm not completely convinced with those test as they need an opam system installed with cli versioning, I'd prefer not to depend on that (even if it will be the usual case).

@dra27
Copy link
Member

dra27 commented Jan 7, 2021

LGTM, thank you!

Note that OPAMCLI=2.1 should never go in a released file (apart from anything else, it generates a warning). All new package descriptions should expect to use --cli=2.1

@dra27
Copy link
Member

dra27 commented Jan 7, 2021

I haven't reviewed the reftests PR yet - do they use the system-installed opam at any point rather than the one just built?

@rjbou
Copy link
Collaborator

rjbou commented Jan 7, 2021

@dra27 I don't think. In reftests, it's the newly compiled opam that is used to launch command to test, a substitution is done. But in test added in this PR, I want to test an opam call inside an opam package, and these call use the system installed opam. The requirement for the installed opam is to have cli versioning working because test output is based on its error messages.

@kit-ty-kate
Copy link
Member Author

I've changed the tests. Feel free to add more if you feel like it should but as is I think this should be good to merge.

@AltGr
Copy link
Member

AltGr commented Jan 11, 2021

This seems sensible; my worry is that we really don't want to encourage calls to opam from within opam packages.

@kit-ty-kate
Copy link
Member Author

my worry is that we really don't want to encourage calls to opam from within opam packages.

I think it is way too late to worry about this right now. People started using it this way years ago.
This PR is about adding a sane default to opam cli system and allow hundreds of packages in opam-repository to compile again with opam.2.1.0~beta4 (coming today?)

@avsm
Copy link
Member

avsm commented Jan 11, 2021

I agree with @kit-ty-kate -- it's use is common (even though we want to discourage it). I'm in favour of this change.

@avsm
Copy link
Member

avsm commented Jan 11, 2021

Just one note: this definitely needs to be documented in the manual somewhere.

@kit-ty-kate
Copy link
Member Author

Good point. There was no section documenting the default environment variables so I've documented this in #4496. (separately because I'd rather avoid blocking this PR and the release on documentation)

@dra27
Copy link
Member

dra27 commented Jan 12, 2021

I also agree - the stable door is definitely off its hinges! CLI versioning and configuration information in the build are orthogonal features (even if CLI versioning does make using the opam command in build scripts less brittle)

@dra27 dra27 merged commit 724330c into ocaml:master Jan 12, 2021
@dra27
Copy link
Member

dra27 commented Jan 12, 2021

Thanks both for the idea and the PR, @kit-ty-kate!

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.

[opam-2.1~beta4] Automatically add OPAMCLI=2.0 when package has opam-version: "2.0"
5 participants