fix(pkg): handle empty package version

[emillon]

Fixes #9176

Due how we expand findlib variables, Some "" can come out of Findlib.Package.Version. We transform this to None before converting to Package_version.t.

[emillon]

This is caused by defaulting to "" in here:

dune/src/dune_findlib/vars.ml

Lines 7 to 10 in 79e656d

	let get (t : t) var preds =
	Option.map (String.Map.find t var) ~f:(fun r ->
	Option.value ~default:"" (Rules.interpret r ~preds))
	;;

But I suppose that we rely on this behavior somewhere.

[Alizter]

Where do we rely on this behaviour?

[rgrinberg]

Do you think you could add a regression test for this?

[gridbugs]

Out of curiosity, how does a package trigger the bug fixed by this PR? Are they explicitly trying to find version "" of a package?

[emillon]

Out of curiosity, how does a package trigger the bug fixed by this PR? Are they explicitly trying to find version "" of a package?

This happens when building the package DB. All versions are computed and they are being returned as "".

Where do we rely on this behaviour?
Do you think you could add a regression test for this?

I agree we have a testing blind spot here since modifying code in that area causes no failures. I'll try to think of a way to test that, that will probably mean writing META files by hand.

[rgrinberg]

Do we need a CHANGES entry for the original change?

[emillon]

This is the first release we introduce Package_version.t so I can't think of another visible change besides dune-build-info.

[rgrinberg]

so I can't think of another visible change besides dune-build-info.

Right, this is what I referred to. I would mention it.

[emillon]

Ah do you mean for #9060? I'm not sure if it changes anything for users.
OTOH I'm wondering how we are going to tighten the Package_version.t if we want to make it completely equivalent to opam version numbers. We can add a versioned check in the future I suppose but I'm not sure it's a property of the dune project, strictly speaking.

[rgrinberg]

We can start with that and just forbid invalid version in 4.0. I don't think anyone will care since such packages wouldn't be submittable to opam anyway

[emillon]

Right, this is what I referred to. I would mention it.

Not sure if you've seen it, but I added a changelog entry for this. Do you think something is missing?

We can start with that and just forbid invalid version in 4.0. I don't think anyone will care since such packages wouldn't be submittable to opam anyway

I'm thinking of cases where the findlib metadata of dependencies (that do not use Dune) is not compatible with opam's format. These can slip through and might exist on opam (though I think we would have caught them in the alpha2 run), and would cause an issue when locking possibly.

[emillon]

(I'm fine with not dealing with this right now)

[rgrinberg]

Yes, I missed the CHANGES entry.

I'm thinking of cases where the findlib metadata of dependencies (that do not use Dune) is not compatible with opam's format. These can slip through and might exist on opam (though I think we would have caught them in the alpha2 run), and would cause an issue when locking possibly.

I've seen people get confused by versions in the META diverging from what opam is reporting. So if anything this is just an an unfortunate reality.

I agree, let's defer this for now.