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

Allow opam source to run without a switch, add a `--no-switch' option #4850

Merged
merged 4 commits into from
May 2, 2022

Conversation

zapashcanon
Copy link
Contributor

@zapashcanon zapashcanon commented Sep 30, 2021

@rjbou Update:


Here's a draft to fix #4809 as asked by @dra27.

It doesn't typecheck:

$ make
dune build --profile=release --root .  --promote-install-files -- opam-installer.install opam.install
File "src/client/opamCommands.ml", line 3358, characters 44-45:
3358 |           OpamAction.prepare_package_source t nv dir @@| function
                                                   ^
Error: This expression has type
         OpamStateTypes.unlocked OpamStateTypes.switch_state
       but an expression was expected of type
         OpamStateTypes.rw OpamStateTypes.switch_state
       Type
         OpamStateTypes.unlocked = [ `Lock_none | `Lock_read | `Lock_write ]
       is not compatible with type OpamStateTypes.rw = [ `Lock_write ]
       The second variant type does not allow tag(s) `Lock_none, `Lock_read

But currently, there's no simple way to get a lock for the virtual state so I don't know how to fix this...

cc @AltGr @rjbou

@rjbou rjbou added the PR: WIP Not for merge at this stage label Sep 30, 2021
@zapashcanon zapashcanon marked this pull request as draft September 30, 2021 12:17
@rjbou rjbou self-assigned this Oct 5, 2021
@rjbou rjbou marked this pull request as ready for review October 13, 2021 15:47
@rjbou rjbou changed the title [WIP] allow opam source to run without a switch Allow opam source to run without a switch, add a `--no-switch' option Oct 13, 2021
@rjbou rjbou added this to the 2.2.0~alpha milestone Nov 8, 2021
@rjbou rjbou removed the PR: WIP Not for merge at this stage label Nov 8, 2021
@rjbou rjbou requested a review from AltGr November 8, 2021 18:58
@dra27 dra27 mentioned this pull request Dec 17, 2021
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.

Just some cosmetic suggestions but otherwise LGTM (the github "hide whitespace" option was helpful ^^)

src/client/opamCommands.ml Show resolved Hide resolved
src/client/opamCommands.ml Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
master_changes.md Outdated Show resolved Hide resolved
master_changes.md Outdated Show resolved Hide resolved
@zapashcanon
Copy link
Contributor Author

If you (@rjbou or @kit-ty-kate) think it's ready for merge, then I'm fine with it.

@kit-ty-kate
Copy link
Member

@zapashcanon actually, if you have some spare time, could you add some tests to a new tests/reftests/source.test file? I just realized this PR can be tested easily and that opam source is not tested at all

@rjbou
Copy link
Collaborator

rjbou commented Mar 16, 2022

test added

@rjbou rjbou force-pushed the source_without_switch branch 2 times, most recently from 36f0ab2 to 61a17f6 Compare March 16, 2022 16:10
tests/reftests/source.test Outdated Show resolved Hide resolved
@rjbou rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Mar 21, 2022
@rjbou rjbou removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Apr 5, 2022
@rjbou rjbou removed the request for review from AltGr April 30, 2022 14:21
@rjbou rjbou merged commit 9ad928e into ocaml:master May 2, 2022
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.

support opam source --no-switch opam source could work without a switch
3 participants