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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
May 13, 2024
Merged

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

merged 1 commit into from
May 13, 2024

Conversation

moyodiallo
Copy link
Contributor

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.

kit-ty-kate
kit-ty-kate reviewed Apr 23, 2024
View reviewed changes
Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

I like this approach, once the code is fixed and works it should be good to go for me at least

src/client/opamClient.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
@kit-ty-kate kit-ty-kate added this to the 2.2.0~beta3 milestone Apr 23, 2024
@moyodiallo moyodiallo marked this pull request as ready for review April 24, 2024 10:49
@moyodiallo moyodiallo requested a review from dra27 April 24, 2024 10:50
@dra27
Copy link
Member

dra27 commented Apr 24, 2024

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
Copy link
Contributor Author

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.

dra27
dra27 reviewed Apr 25, 2024
View reviewed changes
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.

I'm not sure that the code grouping with vflag is a compelling reason to guide the CLI, but it occurred to me on the ride in this morning that my suggestion is not good for a different reason: if --cygwin-internal-install takes an optional argument then opam init --cygwin-internal-install git+https://github.com/ocaml/opam-repository.git changes meaning, and that's bad.

My original issue, as ever, has a bad name suggestion... --cygwin-packages implies we are suggesting the full list of packages, where we are in reality specifying additional packages. Perhaps --cygwin-extra-packages is a better name?

src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamClient.ml Show resolved Hide resolved
@kit-ty-kate
Copy link
Member

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

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

@moyodiallo
Copy link
Contributor Author

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 moyodiallo force-pushed the cygwin-packages-cli-option branch 2 times, most recently from c6bb149 to 1522e27 Compare April 25, 2024 10:17
kit-ty-kate
kit-ty-kate approved these changes Apr 25, 2024
View reviewed changes
Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

LGTM

master_changes.md Outdated Show resolved Hide resolved
dra27
dra27 reviewed May 2, 2024
View reviewed changes
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.

A few typos, but otherwise looks good to me, thanks

src/client/opamClient.ml Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
@moyodiallo moyodiallo force-pushed the cygwin-packages-cli-option branch 2 times, most recently from 4b715ec to bb54890 Compare May 2, 2024 11:07
@rjbou rjbou self-requested a review May 2, 2024 13:35
rjbou
rjbou reviewed May 3, 2024
View reviewed changes
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Thanks! I'm posted few comments, most of them are implemented in separate commits to squash once validated. Remain the behaviour change question.

src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
@rjbou rjbou force-pushed the cygwin-packages-cli-option branch from 86ec788 to d775c21 Compare May 3, 2024 10:42
@kit-ty-kate kit-ty-kate requested a review from rjbou May 8, 2024 11:34
rjbou
rjbou reviewed May 13, 2024
View reviewed changes
src/client/opamCommands.ml Outdated Show resolved Hide resolved
rjbou
rjbou approved these changes May 13, 2024
View reviewed changes
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Except the proposal, LGTM! Thanks!

@moyodiallo
Copy link
Contributor Author

moyodiallo commented May 13, 2024 •
edited
Loading

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

@rjbou
Copy link
Collaborator

rjbou commented May 13, 2024

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 kit-ty-kate mentioned this pull request May 13, 2024
Closed
@kit-ty-kate
Copy link
Member

worked fine. It suffers from #5839 and #5952 but those are separate issues. Thanks a lot!

@kit-ty-kate kit-ty-kate merged commit 036e26f into ocaml:master May 13, 2024
29 checks passed
@dra27 dra27 mentioned this pull request May 18, 2024
Merged
@moyodiallo moyodiallo deleted the cygwin-packages-cli-option branch May 24, 2024 08:26
@dra27 dra27 mentioned this pull request Jun 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dra27 dra27 dra27 left review comments

@kit-ty-kate kit-ty-kate kit-ty-kate approved these changes

@rjbou rjbou rjbou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.2.0~beta3
Development

Successfully merging this pull request may close these issues.

Add CLI option to specify additional packages for internal Cygwin at opam init
4 participants
@moyodiallo @dra27 @kit-ty-kate @rjbou