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.