Various eval-variables fixes and sys-ocaml-system
#5829
Conversation
kit-ty-kate
force-pushed
the
eval-variables
branch
2 times, most recently
from
ae8947b
to
42b76a0
Compare
|
Thanks for sorting out the tests, @kit-ty-kate. I’m happy with the filtering out, unless it would be better just to sed off the values (i.e. be able to see that the variables were defined, but not worry about their differing values), @rjbou? I think the only remaining thing is to put the various definitions into OpamEnv and reference them from there in both OpamFormatUpgrade and OpamInitDefaults. |
How does 01576d0 sound? |
| let env = raw_env () in | ||
| let cyg_env ~env ~cygbin ~git_location = |
There was a problem hiding this comment.
Is there a reason to not have env as an optional argument ? It permit to lighten the call, and let the ability to not decide when dev don't know. All non-cygwin specific calls uses anyway OpamStd.Env.raw_env
| let env = raw_env () in | |
| let cyg_env ~env ~cygbin ~git_location = | |
| let cyg_env ?env ~cygbin ~git_location = | |
| let env = Option.Op.(env +! raw_env ()) in |
There was a problem hiding this comment.
My rationale was that the dev should know 🙂 The exact environment in use at any given point can be quite tricky to determine, so I was figuring that being explicit was potentially better. I don't overly mind, though.
|
I've pushed all my review changes, separated in tiny commits. Once no more changes are needed, I'll do a final commit history cleaning. |
src/core/opamProcess.ml
Outdated
| | Some cygbin -> | ||
| OpamStd.Env.cyg_env ~cygbin | ||
| ~git_location:OpamCoreConfig.(!r.git_location) () | ||
| | None -> OpamStd.Env.raw_env () |
There was a problem hiding this comment.
I don't mind, but the thing I did like before is that it tried to make it clearer that we are always looking at OpamStd.Env.raw_env (), it's just whether it's had cygbin/git_location added to it.
| let env = raw_env () in | ||
| let cyg_env ~env ~cygbin ~git_location = |
There was a problem hiding this comment.
My rationale was that the dev should know 🙂 The exact environment in use at any given point can be quite tricky to determine, so I was figuring that being explicit was potentially better. I don't overly mind, though.
This has the benefit of making them work without requiring Cygwin, but the tiny caveat that they won't work for OCaml < 4.08 and it means that Windows has a different default opamrc since eval-variables can't be filtered. Neither of these things feels like a problem. Co-authored-by: Raja Boujbel <[email protected]>
…efaults et state/OpamFormatUpgrade Co-authored-by: Kate <[email protected]>
eval-variables are in opamrc from the 2.x de-OCaml-ing of opam, but they're conceptually in the wrong place (they should be declared in the repository where they can be synchronised and upgraded). This is a problem for the new variables added in 2.1, because they don't get added to the config of existing roots. This alters the format upgrade to update pre-existing variables and add any new ones. Obviously, if the user had initialised with a different opamrc then we're overwriting that, but that is the conceptual weakness of the field being in opamrc.
Forward planning for the rest of the base-system- packages
Fair enough! I kept the I've updated the PR:
|
|
It's fine thing to have a single PR number with all three of us as authors against it 🙂 Approving the bits I didn't write, thank you! |
|
Thanks! |
#3900 introduced
sys-ocaml-cc,sys-ocaml-libcandsys-ocaml-archwhich followed from some very early demonstrations I gave of the Windows opam packaging proposals at MSR Cambridge in 2015.These variables have been quietly sat on new opam 2.1 installations since 2021. This PR addresses three issues:
I've therefore:
eval-variablesinopamrcis a mistake - they belong to the repository, not to the configuration, but we are where we are and that's something we can look at improving in opam 3.0-config-var, system compilers must be 4.08+ on Windows, but I think this will be fine (it's not like we have old distributions to worry about here - system compilers will have been set-up on purpose to avoid recompilation).Finally, while doing the Windows compiler packaging updates, the variables added in #3900 are working beautifully (as expected), but I propose adding
sys-ocaml-systemso that at some point similar support can be extended to Unix system compilers.sys-ocaml-systemis simply the value ofsystemfromocamlc -configand permits precise detection of the system compiler, especially in a mixed-architecture environment. It's in the same kind of area as #5587 - trying to probe the compiler itself, rather than the present very weak assumption of global variables correlating with all runnable compilers (remembering that an arm64 Windows machine is capable of running 4 different architectures...)