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

Use Dune_lang.Package_name.Opam_compatible.t for package names in package management code. #9371

Closed
gridbugs opened this issue Dec 4, 2023 · 12 comments

Comments

@gridbugs
Copy link
Collaborator

gridbugs commented Dec 4, 2023

It's possible to use invalid opam package names for dune package names in general (e.g. foo.0.1). When interoperating with opam we enforce opam's package naming rules but this is done inconsistently in our package management code as not all operations need to call into opam. It would be a more consistent user experience if all package management related operations require that package names be compatible with opam. The type Dune_lang.Package_name.Opam_compatible.t requires that the name be opam-compatible when values are constructed, so I think this is the type we should use to represent package names rather than the Dune_lang.Package_name.t type we currently use which doesn't require opam-compatibility.

@Alizter
Copy link
Collaborator

Alizter commented Dec 4, 2023

@Alizter
Copy link
Collaborator

Alizter commented Dec 4, 2023

Wouldn't this be better solved by adding more validation to Package_name.t? I'm not aware that we intended it to differ from opam's restrictions.

@gridbugs
Copy link
Collaborator Author

gridbugs commented Dec 4, 2023

I agree that would be better but is there a risk that someone is depending on the current behaviour? I guess we could add the additional checks with a version check. @emillon I see you added Package_name.Opam_compatible quite recently. Is there a reason you added the new type rather than adding checks to the existing one?

@emillon
Copy link
Collaborator

emillon commented Dec 4, 2023

I think I wanted to leverage StringLike to get better error messages and hints, but yes at the moment I don't think that the separate type is used.

@rgrinberg
Copy link
Member

I agree that would be better but is there a risk that someone is depending on the current behaviour?

It could break people's projects, but given that the old behavior is useless anyway and no packages can be submitted to opam without a valid name, I don't see any issue with doing it.

How about we just export the type equality between our own Package_name.t and opam's? All this wrapping and unwrapping we currently have is really not buying us much in my opinion. We can still have our own proper validation on top. There's no contradiction here.

@emillon
Copy link
Collaborator

emillon commented Dec 5, 2023

It could break people's projects, but given that the old behavior is useless anyway and no packages can be submitted to opam without a valid name, I don't see any issue with doing it.

When (generate_opam_files) is set, I agree but there's all the projects which use (package) and public names but without opam files. These can become broken.

@rgrinberg
Copy link
Member

Public names are always validated, and if they are invalid, then they would be rejected by findlib anyway when building something against those packages once they are installed.

Btw, there's no reason to use packages or public names unless the project has components that will get published to opam. So it seems highly unlikely that such packages exist.

Anyway, running an experiment to confirm or deny such suspicions seems like a good next step.

@gridbugs
Copy link
Collaborator Author

gridbugs commented Jan 8, 2024

Seems like it could be hard to find such projects since they won't be on opam. Unifying the types (Package_name.t and Package_name.Opam_compatible.t) and deprecating opam-incompatible package names can wait until 4.0 in my opinion. In the meantime, to get consistent treatment of package names within dune's package management features (as described in the original comment) is there any harm in switching to Dune_lang.Package_name.Opam_compatible.t within our package-management code?

@rgrinberg
Copy link
Member

It would mean that we'd need to juggle 3 different package name types without any (obvious) benefit. What's the difference between Package_name.Opam_compatible.t and OpamPackage.Name.t?

@gridbugs
Copy link
Collaborator Author

gridbugs commented Jan 8, 2024

The only technical reason we can't expose a type equality between Package_name.Opam_compatible.t is that (I assume) we don't want the dune_lang library to depend on opam. I'm not proposing to use Package_name.t alongside Package_name.Opam_compatible.t in our package-management code. I'm proposing to replace the former with the latter. We'd still just be juggling 2 package name types in that code.

@rgrinberg
Copy link
Member

Okay, feel free to send a PR. That shouldn't be too ugly indeed.

gridbugs added a commit to gridbugs/dune that referenced this issue Jan 10, 2024
This refactors package-management code to use
`Dune_lang.Package_name.Opam_compatible.t` for all package names.
Previously we were checking for opam compatibility in some package
management operations but not others leading to inconsistent behaviour.
This removes the inconsistency by making it impossible to represent an
opam incompatible package name in package management code.

Note that there's still an inconsistency in the treatment of package
names in the rest of dune but we can't remove that currently as people
may depend on it. There's a discussion of this problem at:
ocaml#9371

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
gridbugs added a commit to gridbugs/dune that referenced this issue Jan 10, 2024
This refactors package-management code to use
`Dune_lang.Package_name.Opam_compatible.t` for all package names.
Previously we were checking for opam compatibility in some package
management operations but not others leading to inconsistent behaviour.
This removes the inconsistency by making it impossible to represent an
opam incompatible package name in package management code.

Note that there's still an inconsistency in the treatment of package
names in the rest of dune but we can't remove that currently as people
may depend on it. There's a discussion of this problem at:
ocaml#9371

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
gridbugs added a commit to gridbugs/dune that referenced this issue Jan 12, 2024
This refactors package-management code to use
`Dune_lang.Package_name.Opam_compatible.t` for all package names.
Previously we were checking for opam compatibility in some package
management operations but not others leading to inconsistent behaviour.
This removes the inconsistency by making it impossible to represent an
opam incompatible package name in package management code.

Note that there's still an inconsistency in the treatment of package
names in the rest of dune but we can't remove that currently as people
may depend on it. There's a discussion of this problem at:
ocaml#9371

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
gridbugs added a commit to gridbugs/dune that referenced this issue Jan 12, 2024
This refactors package-management code to use
`Dune_lang.Package_name.Opam_compatible.t` for all package names.
Previously we were checking for opam compatibility in some package
management operations but not others leading to inconsistent behaviour.
This removes the inconsistency by making it impossible to represent an
opam incompatible package name in package management code.

Note that there's still an inconsistency in the treatment of package
names in the rest of dune but we can't remove that currently as people
may depend on it. There's a discussion of this problem at:
ocaml#9371

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@gridbugs
Copy link
Collaborator Author

Rather than using Package_name.Opam_compatible.t in package management code we'll instead validate that Package_name.ts are opam-compatible in dune 4.0. That work is tracked by this issue: #9731

@gridbugs gridbugs closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants