Make sure that OPAMROOT and OPAMSWITCH are properly set when executing build commands

[LasseBlaauwbroek]

If one installs a package in a root or switch that is not currently active, and that package calls the opam binary during installation, it will not see the correct root/switch because the environment variables are not set.

Examples:

opam install coq-tactician-stdlib --root=my-root
opam install coq-tactician-stdlib --switch=my-switch

[dra27]

Ouch! Thanks for spotting and fixing this. I think we should fix it more defensively, though:

• OpamConfigCommand.exec does not need optional parameters (all uses of it are labelled)
• OpamEnv.get_opam and OpamEnv.get_opam_raw are never called with optional arguments, so ?set_opamswitch and ?set_opamroot can sensible be made non-optional
• Having done that, the only uses of OpamEnv.get_full with explicit ~set_opamroot and ~set_opamswitch are the ones in this PR: we can either change the default to be true (which I think is better) or do what's here and have OpamEnv.get_full have non-optional arguments too

However, this is a breaking change, albeit a sensible one. It is possible to workaround the present behaviour by explicitly passing --root and --switch to opam calls - i.e. adding "--root" root "--switch" switch to your opam file or using %{root}% and %{switch}% as appropriate.

[LasseBlaauwbroek]

Is this what you propose?

[dra27]

Yes, except for OpamEnv.get_full - I meant that it would either have ?set_opamroot and ?set_opamswitch as now but with ?(set_opamroot=true) and ?(set_opamswitch=true) or just ~set_opamroot and ~set_opamswitch (as for OpamEnv.get_opam_raw)

[LasseBlaauwbroek]

Done

[dra27]

Thanks for the fast updates! This looks fine to me (assuming CI agrees), we just need to determine whether the "breakage" is OK

[rjbou]

I'll take a look to failing tests.

[rjbou]

héhé in fact the PR fixed the test. It was kept on purpose ^^ (to be checked later)

[rjbou]

Updated with a test for this specific case. Otherwise, lgtm! Thanks @LasseBlaauwbroek !

[dra27]

Thanks, @rjbou! We just now need to decide whether this is 2.0+, 2.1+ or 2.2+ 🙂

[LasseBlaauwbroek]

Thanks @rjbou! Regarding versions: It seems to me that If this is breaking, and it is happening in a 2.x, then it might as well be the most recent unreleased one, so 2.1. I would say it is exceedingly unlikely that a package or workflow is relying on an incorrectly set root or switch specifically for opam invocations during builds.

[dra27]

@LasseBlaauwbroek - it is definitely a breaking change! We already have --set-switch and --set-root for opam config and opam exec to control precisely this. I don't think there's a sensible use-case for the old behaviour, but there is the fact that once changed, there are then two different behaviours in released versions.

[LasseBlaauwbroek]

Sure, I agree it is breaking. I'm just suggesting that the break might as well be early then (in 2.1) because the breakage seems exceedingly small and it'll have to break at some point.

[dra27]

Discussed this morning, and approved for 2.0.9 and 2.1, thanks!