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

Implementation of OpamSysPoll in Dune-terms #7868

Merged
merged 1 commit into from Jun 23, 2023

Conversation

Leonidas-from-XIV
Copy link
Collaborator

This branch adds an implementation of the functionality of OpamSysPoll but using our own concurrency primitives.

src/dune_pkg/dune Outdated Show resolved Hide resolved
@rgrinberg
Copy link
Member

Do you want to try using this in the rules? Or in a separate PR?

@Leonidas-from-XIV
Copy link
Collaborator Author

I'll try adding this into the rules!

@rgrinberg
Copy link
Member

Added @gridbugs to review the solver related stuff

@Alizter Alizter self-requested a review June 16, 2023 13:05
src/dune_pkg/opam.ml Show resolved Hide resolved
@@ -52,6 +66,6 @@ module Summary : sig
end

val solve_lock_dir :
repo_selection:Repo_selection.t
repo_selection:Repo_selection.with_sys Repo_selection.t
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to prevent solving packages with repos that don't have system environment variables?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Repo_selection.t is a pure value created by parsing command line arguments, so unfortunately at this point I can't inject the environment variables, since that would require evaluating a fiber. Therefore the injection has to be done in a later step. Hence why I added with_sys_env that (in an admittedly rather ugly way) patches in the env later (where possible). But that means that Repo_selection.t with variables and without would be indistinguishable. To alleviate this I introduced a phantom type to force the user to call with_sys_env to turn an empty Repo_selection.t into a with_sys Repo_selection.t.

But a better solution would be more than welcome, because I am rather unhappy with the code as it is now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing I would like to preserve is the --opam-env=pure behaviour where the solver is run on a completely empty environment for the purpose of reproducable tests. Also I was thinking of at some point letting the user specify variables to set and I think it would be useful to give the option to set variables starting from a totally empty environment.

src/dune_pkg/opam.mli Outdated Show resolved Hide resolved
@Leonidas-from-XIV
Copy link
Collaborator Author

I've rebased again and squashed the commits. Anything missing still or is this ready to go?

Signed-off-by: Marek Kubica <marek@tarides.com>
@rgrinberg rgrinberg merged commit e357e8f into ocaml:main Jun 23, 2023
20 of 21 checks passed
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.

None yet

4 participants