Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stdlib migration issues #2328

Closed
6 tasks done
h-vetinari opened this issue Mar 26, 2024 · 8 comments
Closed
6 tasks done

Stdlib migration issues #2328

h-vetinari opened this issue Mar 26, 2024 · 8 comments

Comments

@h-vetinari
Copy link
Contributor

h-vetinari commented Mar 26, 2024

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

A larger point of discussion came up in #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
Copy link
Contributor Author

Another example of an output gaining a stdlib-dependence spuriously is in conda-forge/brial-feedstock#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
Copy link
Member

isuruf commented Mar 27, 2024

Tying into the discussion from #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
Copy link
Contributor Author

h-vetinari commented Mar 27, 2024

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
Copy link
Contributor

Copying my comment from #2327 (comment) 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
Copy link
Contributor

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
Copy link
Contributor

(N.B.: Didn't see #2330 before; at least from the PR's title, this sounds good.)

@h-vetinari
Copy link
Contributor Author

Small improvement for better detection of sysroot_{{ ... }} landed in 7bb8968

@h-vetinari
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants