Make dune fail when trying to build empty packages (fixes #2882)

[kit-ty-kate]

This is a WIP fix for the long standing issue #2882.
I don't have too much time to spend on this but I wanted to get the ball rolling before the 3.0 release.

Dune has quite a large codebase and I don't really understand much of it. This PR works but the code is rather unpollished.

What's left to be done:

• I couldn't find a better interface for Install.Metadata.t. The current PR makes using of it quite a heavy thing for something that should only be a metadata. If someone has a better idea please feel free to push into this branch.
• I haven't implemented the (may_be_empty) stanza (to add to the (package ...) stanza in dune-project files) discussed in Raise an error when building a defined but empty package #2882
• I haven't implemented the Warning/Error distinction based on the dune-lang version yet (that should be easy)

From the implementation discussion in #2882 I left out the part where enable_if should be left out in the check in favour of a much simpler (more dynamic) check. Two reasons for this change:

• It is much simpler to implement and I do not have the time nor the knowledge of the dune codebase to do that.
• The check in the first place is here to help users discover they're installing an empty package (in practice this only happens when people forget to use (public_name ...). In particular this check would both for users to be warn before releasing in opam-repository and for opam-repository CI. Ignoring enable_if would not enable opam-repository CI to detect that. For instance it wouldn't detect simple typo that would produce an empty package such as:

(library
  (enabled_if (= %{system} "unixx"))
  (public_name test))

[ghost]

What is the rationale behind Install.Metadata.t?

[kit-ty-kate]

What is the rationale behind Install.Metadata.t?

The rational is that for every entry I need to make a difference between:

• "entries added (more-or-less) explicitly by users" (e.g. through (public_name ...) or (install ...))
• "entries added by dune on every packages, even empty ones" (e.g. lib/META, lib/dune-package, lib/opam, share/README, …)

With this metadata I can then filter off all the "entries automatically added by dune" and be left with a simple check: has the user explicitly asked for any files to be installed. If not, print a warning/error.

I couldn't find the way to add it to Entry.t itself because there are functions that construct it and where i have no idea where they are used from, so reconstructing a hypothetical metadata field from this felt harder than simply having type to wrap Entry.t used in the creating of the .install file.

[ghost]

That makes sense

[ghost]

The wrapping seems acceptable to me. However, the Metadata.t type is never used on its own but always used as Loc.t option * _ Metadata.t, so you could fold the Loc.t option inside Metadata.t to get a more compact memory representation.

I'm not sure about the name Metadata btw, it's adding more vocabulary to the Install module for something that's pretty similar to an Entry.t. You could use something like Rich_entry or Entry_ex.

Ignoring enabled_if for this check seems right to me.

[emillon]

It looks like this pattern can be simplified to a List.exists. It might be worth extracting is_user_defined into Install.Metadata too.

[emillon]

This pattern is repeated several times, I suggest creating a Install.Metadata.entry function

[emillon]

I wonder if this could be just a defined_by : Default | User of loc field in Entry.t?

[rgrinberg]

The Entry.t type is used to represent parsed .install file. So this defined_by field would need to be optional or some junk value in this case.

Still the current representation is a little too generic and inconvenient, I'm thinking of replacing it with:

module Entry : sig
  module With_source : sig
    type source = Dune | User
    type nonrec t =
       { source : source
       ; entry : t
      }
  end
end

[rgrinberg]

@kit-ty-kate do you plan to wrap this up soon?

[rgrinberg]

@jeremiedimino are we ok with this being a warning for pre 3.0? The only way for users to address this warning is to upgrade to 3.0.

[ghost]

In this case, I would say yes. From what @kit-ty-kate was saying, this seems to be a common mistake that is only detected quite late. And really having an empty package seems quite rare. So we are helping users more than anything by making it a warning pre 3.0.

However, it'd be better if we could provide a hint to help users mover forward.

[rgrinberg]

Okay, got it. I just want to be very careful when we deviate from our official policy.

I will also rename the option to allow_empty.

[rgrinberg]

@jeremiedimino could you review? This is ready.

[ghost]

The implem looks good but all these warnings scattered all over the test suite are distracting. It'd be good to add (allow_empty) in the various dune-project files of the tests.