Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Write CEP about virtual packages #103
base: main
Are you sure you want to change the base?
Write CEP about virtual packages #103
Changes from 4 commits
ebcfa8e
cda0a8c
6f2d60f
9a793b9
9b4b7a7
59462ed
cedde6a
df9d61d
72022f4
7c3b595
e6fcda5
40b4442
bc6d6a6
5bf10f1
7a2c7e5
cd06910
e520336
1b01182
2148654
eaeb222
9a95c6b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The microarch-level packages in conda-forge depend on
__archspec
to provide microarchitecture-level (e.g. x86-64-v2) meta-packages.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we aware of any existing packages that actually depend on
__archspec
?If not, I would rather we not make this virtual package mandatory at this time, mostly because "what microarchitecture is this CPU?" is a question that can get complicated quickly; see, e.g., ARM big.LITTLE, Intel P/E cores, Intel Xeon 6, etc. To me, this virtual package is still (pseudo-)experimental, in the sense that we still need to work out how package maintainers should/want to use this package. IMO,
__archspec
should be its own CEP or set of CEPs (cf. #59).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#103 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are using
MUST
here but useMAY
in the top level section about environment variables.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the general section covers all cases (and some virtual packages are not overridable), so we can only use
MAY
there. Then we go case-by-case specifying theMUST
parts. Happy to reword for clarity if needed, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cuDriverGetVersion
returns the version of the installed driver, not the oldest CUDA version supported by the detected drivers. Due to backwards compatibility support newer drivers also support older versions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this returns the CUDA version as seen by the driver. The docs are not super clear. There's also
cudaRuntimeGetVersion
At least this is whatconda/conda
uses.@jakirkham could you help clarify this? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think MUSL should be its own thing. It is versioned differently. Plus users presumably want to distinguish GLIBC built packages from MUSL ones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the
stdlib
infrastructure, we have the technical requirements in place to do something like:Whether we want to roll out a "conda-forge for musl" is of course a separate question.
For the purpose of this CEP, definitely
__glibc
needs to stay musl-free IMO. For future-proofing, we could add__musl
, but no strong feelings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the GLIBC version in MUSL distros, not the MUSL version itself. Happy to adjust the wording though. See this block in the constructor SH templates for some more context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having glibc in musl is certainly not the default, I actually didn't know this is supported - TIL. In that case I agree it makes sense to report the glibc that's found.
So my point was that we could also build musl-native libs, but I guess that's much less urgent if alpine-users can just install a glibc compat (assuming this works with our glibc-flavoured packages).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the original comment ( conda/constructor#850 (comment) ), think that user is referring to
gcompat
:While that can be useful, think we should not encode that in this CEP
It may make sense to have a CEP extending this for MUSL (perhaps including
gcompat
) and this could be one detail we include there, but would suggest we leave it out of this version and save it as future workThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the original report was about
gcompat
, but about this alpine-focusedglibc
version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The challenge is if we include this line (even as an example) future readers will look at this line and say "Hey that is how they said we should be supporting MUSL." Even if that is not what we meant. So taking this line out is safer than keep it IMO
The rest of the examples above seem fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at the alpine wiki link I posted:
gcompat is one of them, but there are others.
I don't think that's realistic; it's also easy to preempt - just say "the version of glibc, if present (musl-based distros like alpine only provide this for compatibility; their main C library is musl itself)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry Axel think we had posted around the same time. Agree there can be other ways people try to handle this need
To loop back to Jaime's original point on how to word this, what if we just say the following
Note: Some distros put
libc.so
in different places. So this is worth stating in its own rightThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworded these paragraphs a bit including the intent of the suggestions, let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than list the specific target platforms now and having to adjust this CEP if/when new platforms are adopted, I would suggest language like "when the target platform is sufficiently POSIX-y" and list the attributes necessary for that to be true (e.g., uses
/
for path delimiters, supportsfork(3)
, etc.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the sentiment, but I'm going to need some help or references to compile that list 😬