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

Deprecate using cli version already deprecated build-doc', build-test', and `make' #4581

Merged
merged 4 commits into from
Apr 14, 2021

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Mar 4, 2021

No description provided.

@rjbou rjbou added this to the 2.1.0~rc milestone Mar 4, 2021
@rjbou rjbou requested a review from dra27 March 4, 2021 16:53
@dra27
Copy link
Member

dra27 commented Mar 5, 2021

This doesn't appear to be working correctly:

$ ./opam install bitmasks --build-doc
opam: build-doc was removed in version 2.1 of the opam CLI, but version 2.1 has been requested. Use --with-doc instead or set OPAMCLI environment variable to 2.0.

@dra27
Copy link
Member

dra27 commented Mar 5, 2021

Ouch, there's a more serious problem as well (not necessarily introduced with this PR, but in CLI versioning): OPAMBUILDDOC=true ./opam install bitmasks silently "works" to select the with-doc dependencies of bitmasks.

@rjbou
Copy link
Collaborator Author

rjbou commented Mar 5, 2021

There is no handling of environment variables in cli versioning, only cli.

@dra27
Copy link
Member

dra27 commented Mar 5, 2021

This is an oversight/error for options which have been removed - it shouldn't be that --build-doc is banned but you can quietly persuade opam to do it with an "old" environment variable!

@rjbou
Copy link
Collaborator Author

rjbou commented Mar 5, 2021

I think at that time, it was removed from doc, but kept handled to not break scripts. We can set up a new guidelines for environment variables deprecation.

@rjbou rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Mar 5, 2021
@rjbou
Copy link
Collaborator Author

rjbou commented Mar 5, 2021

Queued, to rebase on cli versioning environment variables edition

@rjbou rjbou removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Apr 6, 2021
Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

LGTM!

@dra27 dra27 merged commit b73ac54 into ocaml:master Apr 14, 2021
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.

2 participants