search

ocaml / opam

opam is a source-based package manager. It supports multiple simultaneous compiler installations, flexible package constraints, and a Git-friendly development workflow.
https://opam.ocaml.org
Other
1.21k stars 348 forks source link

Add CLI option to specify additional packages for internal Cygwin. #5930

Closed moyodiallo closed 2 months ago

moyodiallo commented 2 months ago

Fixes #5834.

Instead of reusing --cygwin-internal-install, it is more suitable to have new option (--cygwin-packages=CYGWIN_PACKAGES), to avoid ending up checking the exclusion between --cygwin-internal-install and --cygwin-local-install.

dra27 commented 2 months ago

I'm not sure what I'm missing, but why do we need a new constructor instead of just adding the list to `internal? Similarly, I'm not sure why having to display an error with --cygwin-packages=CYGWIN_PACKAGES specified on it own is better than --cygwin-internal-install=[CYGWIN_PACKAGES]?

moyodiallo commented 2 months ago

I'm not sure what I'm missing, but why do we need a new constructor instead of just adding the list to `internal? Similarly, I'm not sure why having to display an error with --cygwin-packages=CYGWIN_PACKAGES specified on it own is better than --cygwin-internal-install=[CYGWIN_PACKAGES]?

--cygwin-internal-install, --cygwin-local-install flags are already grouped together and we could let that as it is. Doing --cygwin-internal-install=[CYGWIN_PACKAGES] is going split them and make them separate options.

kit-ty-kate commented 2 months ago

Perhaps --cygwin-extra-packages is a better name?

While it's a bit longer to write, i think i agree with that.

moyodiallo commented 2 months ago

Perhaps --cygwin-extra-packages is a better name?

While it's a bit longer to write, i think i agree with that.

Yep, It brings more context.

moyodiallo commented 2 months ago

I haven't seen no mention about testing it. Did someone test it ?

rjbou commented 2 months ago

I forgot to note but I've tested it when I reviewed installing packages (non installed, already installed, unknown) and some argument incompatibility. @kit-ty-kate is doing further tests.

kit-ty-kate commented 2 months ago

worked fine. It suffers from https://github.com/ocaml/opam/issues/5839 and https://github.com/ocaml/opam/issues/5952 but those are separate issues. Thanks a lot!