search

regro / cf-scripts

Flagship repo for cf-regro-autotick-bot
Other
46 stars 70 forks source link

Stdlib migration issues #2328

Closed h-vetinari closed 2 months ago

h-vetinari commented 3 months ago

For ease of searching: list of bot PRs (checked first ~100 until pyranha-feedstock)

A larger point of discussion came up in https://github.com/regro/cf-scripts/pull/2327 - in general we only want to add stdlib where there's a compiler already in use. However, there could be cases lurking where feedstocks are "working" right now, but not actually propagating compiler run-exports correctly.

One particular such candidate is having a global build stage and then individual outputs that don't build anything anymore, but just pick pieces out of what the global stage produced. In such a case, if run-exports are not forwarded from the global stage to the outputs, then we would create packages with incorrect constraints, unless we defensively add {{ stdlib("c") }} to those outputs as well (which is what #2135 did).

h-vetinari commented 3 months ago

Another example of an output gaining a stdlib-dependence spuriously is in https://github.com/conda-forge/brial-feedstock/pull/34. Tying into the discussion from #2327, @isuruf says it's wrong. I tend to agree (though I'd say "redundant").

IIUC @isuruf's proposal from #2327 correctly, we should only ever add a {{ stdlib("c") }} where there's already a {{ compiler(...) }} of some sort, and any package that currently uses the compilers incorrectly (i.e. in a way their run-exports don't get picked up) would just have yet more missing metadata.

I can implement this easily, but the downside will IMO be that previously "working" recipes (who didn't have to care about missing the compiler run-exports, because they were essentially always there in a non-trivial environment anyway) will silently produce incorrect metadata. On the other hands, the systems that would be affected are all very old and hardly in use anymore.

In any case, I just wanted to record this discussion in a more visible place than spread across various random PRs.

isuruf commented 3 months ago

Tying into the discussion from https://github.com/regro/cf-scripts/pull/2327, @isuruf says it's wrong. I tend to agree (though I'd say "redundant").

Did you notice that brial is noarch: python? That's why I said it's wrong.

h-vetinari commented 3 months ago

Did you notice that brial is noarch: python? That's why I said it's wrong.

The migrator didn't notice, because it didn't get taught about noarch: python. I've tried to involve you from the get-go in #2135, so it shouldn't come as a surprise that it does not know everything that conda-build does. It's supposed to balance implementation complexity with the number of corner cases being handled satisfactorily. In any case, once you approve/merge #2330, that hunk for brial will disappear upon a bot rerun.

mbargull commented 3 months ago

Copying my comment from https://github.com/regro/cf-scripts/pull/2327#issuecomment-2022077281 over:

but isn't it enough to add stdlib only to places where compiler already exists?

I agree. We shouldn't try to drive-by-fix too many things; there are too many (unintentionally broken or just seemingly broken) cases which deems an approach to address many things by some "simple-ish" modification to fail (and make undesired modifications in some cases). IMHO, we should just look for compiler and possibly sysroot_linux*(mabye __osx_+MACOSX_DEPLOYMENT_TARGET in run/run_constrained?) but nothing more. Meaning, adding a requirements/build section that hasn't been there before shouldn't be needed (unless you'd handle the __osx in run/run_constrained case).

mbargull commented 3 months ago

I think handling the cases incorrectly does much more harm than trying to fix (purportedly) broken recipes.

E.g., the noarch case has been mentioned by Isuru and myself already -- if you add we add a stdlib there, we'd outright create packages not installable on other-than target_platform which is a far greater restriction than a potential mismatched libc version constraint. Meaning, in weighing pros and cons, the cons of over-eagerly adding dependency constraints are greater than (potential) pros of tightening (assumed to be) too loose constraints.

mbargull commented 3 months ago

(N.B.: Didn't see https://github.com/regro/cf-scripts/pull/2330 before; at least from the PR's title, this sounds good.)

h-vetinari commented 2 months ago

Small improvement for better detection of sysroot_{{ ... }} landed in https://github.com/regro/cf-scripts/commit/7bb89680b097a9575045f4ef5e977955a5b5221d

h-vetinari commented 2 months ago

Given that we've been migrating for over a month now, I'm going to close this issue. If you find other things to change in the migrator going forward, please leave a comment here or better: open a new issue and tag me.