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
Fix tests #552
Fix tests #552
Changes from all commits
dc20702
ad767e8
b4df151
b51f964
26b6d6e
059ba34
4fe560d
21960de
284a501
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.
We shouldn't have both
numpy >=2.0.0rc1
andoldest-supported-numpy
but it looks like grayskull doesn't support selectors?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.
which selectors are you talking about? It does support a few of them but in this case they might not be being generated for some reason
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.
Yeah this is
requirements/host
. Don't think we wantoldest-supported-numpy
hereThere 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.
see my comment here ;)
https://github.com/conda/grayskull/pull/552/files#r1721631804
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.
that equal over there is just a syntax sugar, but if you properly compare the
numpy
andoldest-supported-numpy
you will see the selectors :)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.
Ah! Indeed, looking at the generated meta.yaml, I see:
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.
Also not sure why
oldest-supported-numpy
is added torequirements/run
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.
Hi @jakirkham , at first sight it seems odd, indeed, but it comes directly from the requirements in that package
gsw
, it is not something that grayskull adds,https://github.com/TEOS-10/GSW-Python/blob/c3407368ef01d0b3d8e85f8cf7b94abae52bcd25/pyproject.toml#L6
that is a separated package on pypi
https://pypi.org/project/oldest-supported-numpy/
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.
Indeed this is actually for the ciso project that has
oldest-supported-numpy
not only in build-system requirements , but in dependencies as well:gsw
has onlynumpy>=1.21
independencies
(oldest-supported-numpy
is only in the build-system).oldest-supported-numpy
doesn't appear the run requirements in that case.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.
It's ok to have in
pyproject.toml
However what this appears to be testing is whether this is added to
requirements/host
andrequirements/run
. The latter doesn't make senseThere 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.
it is going to appear in the requirements run because
oldest-supported-numpy
is specified in the dependencies section of the pyproject.toml. If it is was only in build section of pyproject.toml that would appear only in host then.It is possible to do a workaround and always remove it from the requirements/run and always let it in host, but I am just not so sure if it covers all scenarios, what do 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.
We should remove
oldest-supported-numpy
whenever it is seen. That's used by wheels specifically to configure builds with specificnumpy
versions (not sure why a wheel would useoldest-supported-numpy
at runtime unless they are some kind of build tool)In any event it doesn't make sense in the Conda case where we configure NumPy versions used in some
conda_build_config.yaml
or other variantsOn the Conda side, we need only add
numpy
(unconstrained) torequirements/host
and copy over anydependencies
value ofnumpy
torequirements/run
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.
ok, gotcha
That is pretty much a new PR to do this workaround as it was not a requirement before.
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.
Gotcha thanks for the context! 🙏
The issue is
oldest-supported-numpy
doesn't exist in Conda. So if Grayskull adds it to recipes, they will be broken. Maintainers will need to know they can clean this out to fix the issueIn any event, we can discuss more in the new issue (thanks for raising! 🙏): #553