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 support for opam depexts #10900

Merged
merged 6 commits into from
Sep 17, 2024
Merged

Conversation

moyodiallo
Copy link
Collaborator

dune will just print a warning message whenever a package fails to build. The warning message would be a list of the depexts dependency without checking if they exist in the current OS or not.

This is one of the way for supporting depexts that mentioned in #9294.

> EOF

Lock, build, and run the executable in the project:
$ dune pkg lock
Copy link
Member

Choose a reason for hiding this comment

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

Instead of relying on the solver, you can just write foo.pkg directly.

@rgrinberg
Copy link
Member

I tweaked the error message a bit. The rest looks good but testing the depexts field in the lock dir shouldn't involve the solver.

@@ -0,0 +1,61 @@
When a package fails to build, dune will print opam depexts warning.
Copy link
Member

Choose a reason for hiding this comment

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

Can you move these two tests into a depexts sub-directory?

moyodiallo and others added 6 commits September 17, 2024 10:16
`dune` will just print a warning message whenever a package fails to build.

Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
@moyodiallo
Copy link
Collaborator Author

Suggestions applied thanks.

@rgrinberg rgrinberg merged commit 4f86203 into ocaml:main Sep 17, 2024
22 of 27 checks passed
@moyodiallo moyodiallo deleted the pkg-depexts branch September 17, 2024 10:40
@anmonteiro
Copy link
Collaborator

we tried this with reason-react and it resulted in a dune crash. check #10930

@anmonteiro
Copy link
Collaborator

this is likely because we use npm-version which is a variable interpreted by an opam-plugin (check https://github.com/reasonml/reason-react/blob/c07e413750c5ca1632e6d3405a3cfbdf8d24c84e/reason-react.opam#L45-L48).

@moyodiallo
Copy link
Collaborator Author

I'm for reverting it until we fix the subsequent bug.

@rgrinberg
Copy link
Member

Do you want to add a repro test to demonstrate the issue before you revert?

@moyodiallo
Copy link
Collaborator Author

A question: is npm-version an opam variable ? I'm not sure which test to add.

@anmonteiro
Copy link
Collaborator

npm-version is interpreted by opam-check-npm-deps

@moyodiallo
Copy link
Collaborator Author

This variable is specific to depexts on my understanding so far. I think the better way to go, is to maintain the PR and fix issue. If anyone has another proposition I'm open to it.

anmonteiro pushed a commit to anmonteiro/dune that referenced this pull request Nov 17, 2024
* pkg: add support for opam depexts

`dune` will just print a warning message whenever a package fails to build.

Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
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.

4 participants