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

Added rudimentary check on package names in dependencies #9385

Closed
wants to merge 0 commits into from

Conversation

ElectreAAS
Copy link
Collaborator

Resolves #9270.

This PR adds a simple check to all the <pkgname> listed in the dependencies of a dune-project.

I just saw #9371, and have no opinions on it, but since they look closely related, I'm linking it here too.

Pinging @Leonidas-from-XIV & @emillon for a review.

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.

Great to have your first PR! I've added a few comments, convenient that there are already some cram tests to test the change.

src/dune_rules/package.ml Outdated Show resolved Hide resolved
test/blackbox-tests/test-cases/invalid-package-version.t Outdated Show resolved Hide resolved
test/blackbox-tests/test-cases/invalid-package-version.t Outdated Show resolved Hide resolved
@ElectreAAS
Copy link
Collaborator Author

Note: I only added the 'opam compatible' check in the depends field, but not the similarly formed conflicts and depopts. For the latter it seems to make sense, but the former?

@Alizter
Copy link
Collaborator

Alizter commented Dec 5, 2023

@ElectreAAS We use conflicts as inputs to the solver IIRC so they should also be validated the same as depends.

@ElectreAAS
Copy link
Collaborator Author

I just rebased the PR on main, sorry for the noise.

I have updated the expect part of test/blackbox-tests/test-cases/pkg/invalid-version.t, but not the text yet, as I'm quite confused as to what is the difference in tested behaviour in this test and test/blackbox-tests/test-cases/invalid-package-version.t.

@Alizter
Copy link
Collaborator

Alizter commented Dec 7, 2023

The two tests are slightly different:

  • One of them is about the fact that during dune build the package depends field is never validated. Allowing invalid names to go into the .opam file generation.
  • The other is about dune lock not giving a good error when such a name is encountered.

@Leonidas-from-XIV
Copy link
Collaborator

However should we care about (depends) if we're not generating opam files and never use the data?

Not sure how many packages specify depends without generating opam files, because without it it is fairly pointless at the moment.

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.

Writing foo.1.2.3 as a package dependency should give a nice error and a hint
3 participants