conda-forge / re2-feedstock

A conda-smithy repository for re2.
BSD 3-Clause "New" or "Revised" License
2 stars 12 forks source link

Circular run-exports lead to overdepending #72

Open h-vetinari opened 10 months ago

h-vetinari commented 10 months ago

With the solution picked for #67 / #68, we need a somewhat circuitous mechanism to avoid that different libre2-{X,Y} versions can be co-installed. In particular, https://github.com/conda-forge/re2-feedstock/blob/4a93f51cf0c0da27ac490cae3ecb4339ff4c7801/recipe/meta.yaml#L35-L44

To recap, re2 depends exactly on libre2-X (which it also run-exports) and libre2-X has a run-constraint on the exact re2 version that was used to build it. It's somewhat unintuitive that this works out as intended but if re2 v2023.a.b and re2 v2024.c.d have the same SOVERSION, a downstream package built against v2023.a.b (at some point in the past) will be able to pull in the newest libre2-X built from v2024.c.d at runtime.

However, the run-constraint is not enough to avoid that builds against different libre2-{X,Y} cannot be installed side-by-side (which we want to avoid due to this), because the constraint only fires if re2 is actually present in the environment.

So additionally, we have a run-export of re2 to re2, that ensures that the run-constraint of libre2-X fires and so all should be well.

However, because re2 doesn't contain the actual libraries, the link-check now starts to complain. This will lead to errors in feedstocks that opt into error_overlinking: true and might confuse maintainers that manually try to keep their link-check clean:

WARNING (libgrpc): run-exports library package conda-forge::re2-2023.06.02-h2873b5e_0 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)

In particular, removing re2 as a host-dependence will not solve the issue for such feedstocks, and worse: if they do the seemingly right thing of

ignore_run_exports:
  - re2

that just brings us back to square one, because that breaks the above mechanism to avoid co-installation.

I think the fix is to elevate the run-constraint to an actual run-dependence of libre2-X on re2, and then remove the re2 run-export from re2, but perhaps I'm missing something.

CC @isuruf @xhochy

h-vetinari commented 10 months ago

I think the fix is to elevate the run-constraint to an actual run-dependence of libre2-X on re2, and then remove the re2 run-export from re2, but perhaps I'm missing something.

Yup, missed the fact that that's not possible because it introduces a build cycle. 😑

isuruf commented 10 months ago

This will lead to errors in feedstocks that opt into error_overlinking: true and might confuse maintainers that manually try to keep their link-check clean:

It does? Do you have an example log?

h-vetinari commented 10 months ago

Do you have an example log?

Yes

isuruf commented 10 months ago

The log you linked to passes fine. You said it will error if error_overlinking: true. Do you have an example of that?

h-vetinari commented 10 months ago

Yes, because that feedstock does not use error_overlinking: true or error_overdepending: true (which are kind of like -Werror for the link check).

Still, the warning is clear from the log, including the nudge to add re2 to ignore_run_exports, which would break the mechanism we have in place.

WARNING (libgrpc): run-exports library package conda-forge::re2-2023.06.02-h2873b5e_0 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)

That's because the link check picks up the output which actually contains the DSO:

   INFO (libgrpc,lib/libgrpc.so.35.0.0): Needed DSO lib/libre2.so.11 found in conda-forge::libre2-11-2023.06.02-h7a70373_0

leaving re2 unused (from its POV).

isuruf commented 10 months ago

All I'm saying is that your statement about error_overlinking: true is false.

h-vetinari commented 10 months ago

OK. Strike that off the record then. The warning still instructs maintainers to do the very thing that will break this mechanism.

isuruf commented 10 months ago

Same can be said of numpy. From scipy-feedstock:

WARNING (scipy): run-exports library package conda-forge::numpy-1.22.4-py310h4ef5377_0 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
h-vetinari commented 10 months ago

All I'm saying is that your statement about error_overlinking: true is false.

Here you go with a failing CI run now:

ERROR (libgrpc): run-exports library package conda-forge::re2-2023.06.02-h2873b5e_0 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)

Same can be said of numpy.

Numpy is different. Quoting @xhochy:

Due to the nature of NumPy's linking mechanism (which is really great!), you should never see an actual linkage against it. If the warning isn't always showing up, I would be very curious why it happens. All references are filled at runtime through the C function import_numpy().

xhochy commented 5 months ago

Should we have instead named it simply libre2 instead of adding the version to the name of the library?

xhochy commented 5 months ago

My proposal would be to have a libre2-mutex package that is

a) empty b) noarch: generic c) has the SOVERSION as the version d) re2 and libre2-SOVERSION depend on it as run dependency.

This should solve the overlinking/overdepending issues as it is not a run_exports but also always present to the solver.

isuruf commented 5 months ago

I think we should just ignore overdepending checks. It's a broken check. Overlinking check should work just fine.

xhochy commented 5 months ago

I find them useful to detect unnecessary dependencies and thus would like to see them working. Looking through the implementation in `conda-build, ' I don't see an easy way to ignore them; therefore, the proposal is with a shared mutex package. With that, we should get around the warning/error of the check and support it also with other build tools (like rattler-build) as it isn't a conda-build-specific workaround.

isuruf commented 5 months ago

I'd like to avoid adding another package to work around a conda-build bug. I think the current recipe here is a good one that we want to generalize to other packages, but having a mutex would make this not elegant.

h-vetinari commented 5 months ago

I think the patterns should be adapted to what they should achieve:

Here we want to avoid co-installability, but use versioned outputs, which to me is upside down. It also requires complicated and almost circular constraints and run-exports, which in itself is not something I want to see spread more widely, because it's hard to grasp and substantially raises the bar for contributions.

All that would still make it somewhat viable, but given the additional false positives for the overdepending-check (which many maintainers do react to; which would in turn undo the constraints that prevent co-installability), I really think this is not something we should propagate.