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

pkg: add cond statement for choosing lockdir #9750

Merged
merged 1 commit into from Jan 23, 2024

Conversation

gridbugs
Copy link
Collaborator

We expect projects with system-specific packages to include multiple lockdirs - one for each system. This change adds a dsl for selecting the appropriate lockdir for the current system.

@gridbugs
Copy link
Collaborator Author

Opening this PR to get feedback on my approach. If there are no major issues with how this works then I'll add some comments for posterity.

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Seems reasonable to have a mechanism to automatically assign the lockdir.

src/dune_rules/expander.ml Outdated Show resolved Hide resolved
@emillon emillon changed the title pkg: add cond statement for chossing lockdir pkg: add cond statement for choosing lockdir Jan 16, 2024
@gridbugs gridbugs force-pushed the lock-directory-selection branch 2 times, most recently from 8be6c22 to a32b309 Compare January 17, 2024 01:53
@rgrinberg
Copy link
Member

Will hopefully review this tomorrow

@rgrinberg
Copy link
Member

The implementation of the syntax looks good, but the data used to expand these pforms is suspicious. Using the compiler from the ambient environment to choose the lock directory is likely to cause unexpected behavior. For example, users aren't expected to have a compiler installed at all to use lock directory, and dune should install the compiler for them. Therefore, I suggest that we limit the pforms to whatever is computable without the compiler. See Sys_poll for such options.

@gridbugs
Copy link
Collaborator Author

I've updated this to implement the pform expander with sys_poll rather than ocamlc -config and added a default case to cond statements with a default keyword.

@gridbugs gridbugs force-pushed the lock-directory-selection branch 2 times, most recently from 5904683 to 7bf021b Compare January 22, 2024 05:07
src/dune_rules/lock_dir.ml Outdated Show resolved Hide resolved
;;

let os = string_option_config "os"
let arch = string_option_config "arch"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just allow overriding these variables at build rime rather than just during solving. That would replace this config and and would likely be a useful feature in general.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should already be the case. The overrides are applied in functions in Sys_poll such as Sys_poll.os which are used to compute variables both at solve time and build time.

We expect projects with system-specific packages to include multiple
lockdirs - one for each system. This change adds a dsl for selecting the
appropriate lockdir for the current system.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@gridbugs gridbugs merged commit 9c32296 into ocaml:main Jan 23, 2024
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants