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.

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

Modified the command to set the --set-switch flag #744

Merged

Conversation

SaySayo
Copy link
Contributor

@SaySayo SaySayo commented Oct 18, 2021

Modified the command to set the --set-switch flag. Fixes (#655). @tmattio @mnxn please review

@rgrinberg
Copy link
Contributor

--set-switch set the switch globally right? If it does, setting OPAMSWITCH might be better.

@Khady
Copy link
Contributor

Khady commented Oct 18, 2021

If it does, setting OPAMSWITCH might be better.

With opam exec the --set-switch flag does chagne OPAMSWITCH

       --set-switch
           With the env and exec subcommands, also sets the OPAMSWITCH
           variable, making sure further calls to opam will use the same
           switch as this one.

src/opam.ml Outdated
Comment on lines 223 to 225
let switch_arg switch = ["--switch=" ^ Switch.name switch; "--set-switch"]

let exec t switch ~args = spawn t ("exec" :: switch_arg switch :: "--" :: args)
let exec t switch ~args = spawn t (("exec" :: switch_arg switch) @ ("--" :: args))
Copy link
Contributor

Choose a reason for hiding this comment

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

the --set-switch flag is only needed on the calls to opam exec. So switch_arg doesn't need to be modified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean I pass the --set-switch flag to only the exec function and remove it from the switch_arg?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Basically the whole PR will become

- let exec t switch ~args = spawn t ("exec" :: switch_arg switch :: "--" :: args)
+ let exec t switch ~args = spawn t ("exec" :: switch_arg switch :: "--set-switch" :: "--" :: args)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Made the corrections.

src/opam.ml Outdated
Comment on lines 268 to 275
spawn t (("install" :: switch_arg switch) @ ("-y" :: packages))

let update t switch = spawn t [ "update"; switch_arg switch ]
let update t switch = spawn t [ "update"; switch_arg switch ] @ ["--switch=" ^ Switch.name switch; "--set-switch"]

let upgrade t switch = spawn t [ "upgrade"; switch_arg switch; "-y" ]
let upgrade t switch = spawn t [ "upgrade"; switch_arg switch; "-y" ] @ ["--switch=" ^ Switch.name switch; "--set-switch"]

let remove t switch packages =
spawn t ("remove" :: switch_arg switch :: "-y" :: packages)
spawn t (("remove" :: switch_arg switch) @ ("-y" :: packages))
Copy link
Contributor

Choose a reason for hiding this comment

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

there's already switch_arg switch so you don't need to insert ["--switch=" ^ Switch.name switch; "--set-switch"] again. But actually for all those commands there's no use for --set-switch so this part of the diff can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted!

@Khady
Copy link
Contributor

Khady commented Oct 20, 2021

I'm not part of the team, so won't be the one validating your work. But thank you for the contribution!

@tmattio
Copy link
Collaborator

tmattio commented Oct 20, 2021

Thanks for the work @SaySayo and the review @Khady 🙂

This looks good to me, but @ulugbekna could you confirm that this is the expected fix for #655?

@ulugbekna ulugbekna force-pushed the Fix-opam-set-switch-in-sandboxed-terminal branch from 3562d78 to 3595a3b Compare October 20, 2021 13:42
@ulugbekna ulugbekna force-pushed the Fix-opam-set-switch-in-sandboxed-terminal branch from 3595a3b to a4e0d05 Compare October 20, 2021 13:44
@ulugbekna ulugbekna merged commit e435f2d into ocamllabs:master Oct 20, 2021
@ulugbekna
Copy link
Collaborator

Thanks, @SaySayo ! :-)

@SaySayo SaySayo deleted the Fix-opam-set-switch-in-sandboxed-terminal branch October 20, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Suggestion] Have sandboxed terminals alias opam to opam --switch=$SWITCH
5 participants