Refactor Dune_lang.Package

[Leonidas-from-XIV]

While working on the bug where we ignore dependencies from OPAM file if there is a package definition in the dune-project when locking, I ran across a weird behavior:

Dune_lang.Package.t contains a lot of fields which have very little to do with parsing a package stanza and is overloaded with tons of stuff that is either only relevant when the Package.t has been created from an OPAM file or read from a dune-project (as evident by the encode/decode functions which ignore a lot of fields and a lot of accessor functions where data is set after the package stanza has been decoded).

This also leads to the strange behavior that an OPAM file is read, a Package.t is created with mostly invalid/empty data, containing the original opam file content as an attachment. When we attempt to lock, we ignore all these invalid fields and parse the opam file again.

This seemed all a bit messy, so in this PR I'm trying to split this up a bit. I've moved the Dune_lang.Package to Dune_lang.Dune_package and created a new wrapper Package type that can be created from either dune-project or an opam file. I generally think that whatever Package is doing should probably be moved to Dune_rules.Package (and Dune_lang.Dune_package should become Dune_lang.Package again) but I first wanted to see how big the impact is on the testsuite. There is a bit of breakage but I am reasonably certain it can be fixed fairly easily.

But before polishing it up I'd like to ask whether this direction is something that makes sense or whether I am missing something crucial and whether it creates a lot of issues. @rgrinberg could you please comment whether the plan makes sense to you?

[rgrinberg]

While working on the bug where we ignore dependencies from OPAM file if there is a package definition in the dune-project when locking

This is not a bug but expected behavior. What behavior did you expect instead?

As for the refactoring, the exact approach depends on features or bug fixes that we'd like to achieve. So it's hard to evaluate without knowing what concrete goal you have in mind. After a brief glance, I don't see anything particularly wrong but neither something that is especially necessary. Probably because it's quite a large change so it's hard for me to evaluate it all at once. Maybe some of the code moves can be done separately?

I will say that a good guiding principle is that this distinction between the types of packages (opam vs dune) is not something to be leaked. It should be something that we isolate to a single place as much as possible, so that the rest of the code isn't burdened with needing to remember all these details. I don't think the current state of the code achieves this goal properly, but a good change will move towards that.

[Leonidas-from-XIV]

This is not a bug but expected behavior. What behavior did you expect instead?

I think in general Dune co-exists with OPAM fairly well, so from a user perspective I would have expected that if there isn't any dependencies declared in dune-project it would use the dependencies declared in <packagename>.opam. Like (package (name foo) (depends)) would mean that I have no dependencies, but (package (name foo)) just doesn't claim anything about dependencies - also not their absence. Same with the other optional fields in (package).

This also seems to be how some project in the wild are set up and it doesn't strike me as unreasonable.

So it's hard to evaluate without knowing what concrete goal you have in mind.

I think my goal is to have a better way to have package definitions coexist at the same time. At the moment this is done in an sort of C-ish union type Project.t which includes some things that can only come from dune-project (e.g. allow_empty), some things that can only come from opam files (complex OPAM dependency formulas) and some things that can't be defined in either (e.g. whether to generate opam files).

This PR changes it into an ADT where a project is either one or the other and will return roughly correct data when both ways do declare a project offer the same information, but also allows to specifically require one of them if the action only makes sense using one of them (val dune_package : Package.t -> Dune_package.t option). Thinking about this over the weekend, a potentially better way would be to always load both (if available) and then allow the code to react on them. E.g. if the dune-project package is declared, but has no dependencies, check if the dependencies are declared in the opam file. Likewise with other fields.

I will say that a good guiding principle is that this distinction between the types of packages (opam vs dune) is not something to be leaked.

I agree, and generally I try to avoid it (e.g. Package.allow_empty always returns false for packages declared from Dune and helps to keep the change a bit smaller as I don't want to change every reference to Package.* if not necessary), but there are cases where there is no representation that is a superset of the other. Dune dependencies and OPAM dependencies are different, e.g. the latter needs to be evaluated to know which packages it refers to.

Probably because it's quite a large change so it's hard for me to evaluate it all at once. Maybe some of the code moves can be done separately?

I can definitely split this up into more digestable sizes if we agree on the direction. Especially as this PR evolved as I tried to get it to compile again and learn what issues crop up.

[rgrinberg]

Like (package (name foo) (depends)) would mean that I have no dependencies, but (package (name foo)) just doesn't claim anything about dependencies - also not their absence. Same with the other optional fields in (package).

I think that encourages users to split their package definitions across the dune and the opam format. In addition to just being confusing, it also requires us to think about resolving conflicts between the metadata defined in both places at once. In general, it's easy to think about packages as being either strictly dune's or strictly opam's. Allowing for chimeras is not to our advantage. It at least shouldn't be done without some strong justification on why it's necessary (beyond convenience)

[Leonidas-from-XIV]

I generally would like to encourage people to migrate their data to the dune-project side, but if they can't express their dependencies in the dependency list format they will probably stick to opam files. As such supporting both and allowing mixing but preferring the data from dune-project in case of conflicts seems to me like an decent way to offer people an "upgrade path".

The alternative is to try to detect the case where there are no dependencies in dune-project but they exist in .opam and fail with the error/warning to please delete package from dune-project. The current behavior of quietly assuming there are no dependencies is rather unintuitive. This would be a much shorter and easier to review PR :)

[rgrinberg]

but if they can't express their dependencies in the dependency list format they will probably stick to opam files. As such supporting both and allowing mixing but preferring the data from dune-project in case of conflicts seems to me like an decent way to offer people an "upgrade path".

but if dune can't express their dependency specification, then dune can't build their dependencies either. So what's the point of moving to the dune-project file?

The alternative is to try to detect the case where there are no dependencies in dune-project but they exist in .opam and fail with the error/warning to please delete package from dune-project. The current behavior of quietly assuming there are no dependencies is rather unintuitive. This would be a much shorter and easier to review PR :)

It's not really about the length but the steering the users to express their intents in a clear way. Chimera mode just creates a whole bunch of intermediate states for us to guess the meaning of. I think we already allow them to upgrade things gradually by transitioning things per package. I think that's accommodative enough.

I do agree that having better warnings and errors is good though. It seems like a logical thing to me never mix and match two sources of truth for a single logical object in the workspace, I just wonder why would users knowingly to obfuscate things this way.

[Leonidas-from-XIV]

but if dune can't express their dependency specification, then dune can't build their dependencies either. So what's the point of moving to the dune-project file?

Only the solver needs to support it (and it does, because we can typically find solutions for opam files). Once the solver has a solution it is just a list of package names and versions.

I do agree that having better warnings and errors is good though.

Ok, fair I'll work on that in #11573.

I just wonder why would users knowingly to obfuscate things this way.

Generally I think it is due to a general lack of understanding what package does. If you add it, it doesn't make a difference but also doesn't make things error out, so people just have somewhat nonsensical project setups sometimes.