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

Start copying commands from opam files #8336

Merged

Conversation

gridbugs
Copy link
Collaborator

@gridbugs gridbugs commented Aug 4, 2023

Very early implementation of copying build and install commands from opam files into lockfiles meant to invite feedback on the general design of this feature before going deeper.

Not yet supported:

  • filters on commands or arguments
  • string interpolation in commands
  • package-scoped variables other than _ (the current package)
  • executing the actions

Not all opam variables are supported yet.

@gridbugs gridbugs force-pushed the basic-converting-commands-from-opam-files branch 4 times, most recently from 0664d72 to 172569a Compare August 8, 2023 05:08
@gridbugs
Copy link
Collaborator Author

gridbugs commented Aug 8, 2023

Updated to use run instead of system. Also it now omits the build and install fields if there are no commands and only uses Progn if there were multiple commands.

Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

LGTM. A few more tests (with more forms) would be useful, but they can always be added later.

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.

I have some small comments about this but overall I think its good and moving to the right direction. Good chances that even without resolving of the package variables this is enough to cover 80% of all OPAM packages.

src/dune_lang/pform.ml Outdated Show resolved Hide resolved
src/dune_pkg/opam_solver.ml Outdated Show resolved Hide resolved
src/dune_pkg/opam_solver.ml Outdated Show resolved Hide resolved
src/dune_pkg/opam_solver.ml Show resolved Hide resolved
@gridbugs
Copy link
Collaborator Author

gridbugs commented Aug 9, 2023

Note that long-term we probably won't be able to use Run actions to implement opam commands due to filters on individual terms. Opam allows filters on the first term of a command, for example:

build: [ "make" {os-family = "linux"} "gmake" {os-family = "freebsd"} ]

Even though opam treats the first term of a command as the program and subsequent terms as arguments, filtering takes place before determining which term is first, so the program can be determined dynamically based on filters. In dune this filtering must happen at build time rather than solve time in order for single lockdirs to support multiple environments. I think we'll need a new construct to handle filters on terms.

This doesn't affect this PR. Just an observation.

@gridbugs gridbugs force-pushed the basic-converting-commands-from-opam-files branch from 172569a to 138ebd1 Compare August 9, 2023 07:51
error messages without quotes. *)
User_error.raise
[ Pp.textf
"Encountered unknown variable \"%s\" while processing commands for \
Copy link
Member

Choose a reason for hiding this comment

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

This can be %S for automatic quoting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

woah surprising, in C it's for strings of wide characters right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure it is standard, I can only find %S in Microsoft reference documents, not sure it also applies to POSIX.

Very early implementation of copying build and install commands from
opam files into lockfiles meant to invite feedback on the general design
of this feature before going deeper.

Not yet supported:

- filters on commands or arguments
- string interpolation in commands
- package-scoped variables other than `_` (the current package)
- executing the actions

Not all opam variables are supported yet.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@rgrinberg rgrinberg force-pushed the basic-converting-commands-from-opam-files branch from 138ebd1 to e7f8c96 Compare August 9, 2023 18:23
@rgrinberg rgrinberg merged commit 758e370 into ocaml:main Aug 9, 2023
21 checks passed
dkalinichenko-js pushed a commit to dkalinichenko-js/dune that referenced this pull request Aug 15, 2023
Very early implementation of copying build and install commands from
opam files into lockfiles meant to invite feedback on the general design
of this feature before going deeper.

Not yet supported:

- filters on commands or arguments
- string interpolation in commands
- package-scoped variables other than `_` (the current package)
- executing the actions

Not all opam variables are supported yet.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
Signed-off-by: Diana Kalinichenko <dkalinichenko@janestreet.com>
@gridbugs gridbugs deleted the basic-converting-commands-from-opam-files branch October 11, 2023 03:48
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

4 participants