DUNE_PROFILE should be ignored when -p specified

[dra27]

DUNE_PROFILE being set in the environment can break the installation of opam packages:

dra@thor:~$ DUNE_PROFILE=dev opam install uri.2.2.1
The following actions will be performed:
  ∗ install uri 2.2.1

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
⬇ retrieved uri.2.2.1  (cached)
[ERROR] The compilation of uri.2.2.1 failed at "dune build -p uri -j 7".

#=== ERROR while compiling uri.2.2.1 ==========================================#
# context     2.1.0~beta4 | linux/x86_64 | ocaml-base-compiler.4.11.2 | file:///home/dra/opam-repository
# path        ~/.opam/dev-4.11/.opam-switch/build/uri.2.2.1
# command     ~/.opam/opam-init/hooks/sandbox.sh build dune build -p uri -j 7
# exit-code   1
# env-file    ~/.opam/log/uri-3421537-40d977.env
# output-file ~/.opam/log/uri-3421537-40d977.out
### output ###
# [...]
# - [@sexp_drop_default.equal] if the type supports [%equal]
# - [@sexp_drop_default.sexp] if you want to compare the sexp representations
#
# File "lib/uri_sexp.ml", line 21, characters 18-31:
# 21 |           scheme: string option [@default None] [@sexp_drop_default];
#                        ^^^^^^^^^^^^^
# Error (warning 22): [@sexp_drop_default] is deprecated: please use one of:
# - [@sexp_drop_default f] and give an explicit equality function ([f = Poly.(=)] corresponds to the old behavior)
# - [@sexp_drop_default.compare] if the type supports [%compare]
# - [@sexp_drop_default.equal] if the type supports [%equal]
# - [@sexp_drop_default.sexp] if you want to compare the sexp representations
#



<><> Error report <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
┌─ The following actions failed
│ λ build uri 2.2.1
└─
╶─ No changes have been performed

--profile got changed as part of the 2.0 bootstrap, so it can be specified with -p (so that the bootstrap can do -p --profile dune-bootstrap. This is fine for the command line, but DUNE_PROFILE should definitely be ignored in this case.

Cmdliner doesn't make the provenance of a term available. There are a few possible fixes:

1. Don't pass DUNE_PROFILE to Cmdliner at all and manually fill it in (this is what opam does for pretty much all of its environment variables - the downside is that the man-page no longer automatically documents the variable). This is a fairly simple change.
2. Put a sentinel value on the start of DUNE_PROFILE before evaluating the term, so that Dune can tell that the environment variable was used. The more serious downside is that it affects the evaluation of the term for all the commands so it's a bigger diff.
3. Only allow dune-release as the argument for --profile when -p is given. Also fairly simple; might break some unexpected usage of dune.
4. Pre-process Sys.argv to identify the presence of -p and anything beginning --profile (this isn't as crazy as it sounds, but it's quite crazy - FWIW we have to pre-process Sys.argv in opam, too!). If -p is present (before any --) and no term begins --profile, the the --profile argument could be omitted in the term.
5. Restore --profile to the pre-2.0 behaviour of conflicting with -p and introduce a different argument for the bootstrap to communicate the bootstrap profile.

Technically speaking I broke this in #2854 (although I think the change was actually Jeremie's 😉) - I'm very happy to do whichever of these fixes we settle on! The options above are given in increasing order of preference (so I think 4 or 5 are probably best - 4 is slightly dirtier internally, but doesn't require a special bootstrap flag which is always visible in the CLI).

[ghost]

Haha, yeah it was definitely me. I remember this.

What about we patch cmdliner to allow capturing the source?

[dra27]

I guess that can be option 6 - but I imagine that such a patch would have to live in Dune, just because it will be an API change for Cmdliner?

[ghost]

The API questions are more for cmdliner upstream. I imagine that if you add a new function to preserve backward compat it should be fine. But if you want to add a patch to ocaml-dune/cmdliner first that's fine as well.

[ghost]

We discussed this during today's meeting. Doing something ad-hoc for DUNE_PROFILE seems fine, although it would be nicer to have a more principled way to support such command line aliases.

@bobot and @aalekseyev had some ideas how to do that.

[dra27]

This is fixed in Dune 2.9.0, thank you!