-
Notifications
You must be signed in to change notification settings - Fork 354
Test to handle package requests that are strings separately #2004
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
base: main
Are you sure you want to change the base?
Test to handle package requests that are strings separately #2004
Conversation
Signed-off-by: Craig Zerouni <[email protected]>
Hey @czerouni , thanks for this PR. I took a quick look and I feel like this can be fixed in a cleaner way. I think we could add Lines 74 to 76 in fca0b86
I tested quickly locally with a recipe like this: name = "test_package"
version = "1.0.1"
description = "Something `rez-env`-ing this"
@late()
def private_build_requires():
return ["os-arch"]
build_command = "python -c 'print(1)'"
def commands():
env.ASD.append('~asdasdads') and this snippet seems to be working: >>> import rez.packages
>>> package = rez.packages.get_latest_package('test_package')
>>> variant = list(package.iter_variants())[0]
>>> print(variant.get_requires(private_build_requires=True))
[PackageRequest('os-arch')] Could you give this a go and see if the tests pass? |
Hi. I'm sorry, I am not understanding what you mean. These lines
are already in |
I mean this: late_bind_schemas = {
- "requires": late_requires_schema
+ "requires": late_requires_schema,
+ "build_requires": late_requires_schema,
+ "private_build_requires": late_requires_schema,
} |
Yes, that seems to accomplish the same thing. Do you want me to close this PR? Or maybe change it to your fix? |
You can push a commit on this branch and keep the same PR |
…and private_build_requires Signed-off-by: Craig Zerouni <[email protected]>
All set! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2004 +/- ##
=======================================
Coverage 60.01% 60.01%
=======================================
Files 163 163
Lines 20098 20098
Branches 3494 3494
=======================================
Hits 12062 12062
+ Misses 7224 7217 -7
- Partials 812 819 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Is this expected or is it related to my changes? |
Everything looks good to me. The codecov report was created because I ran the CI. The macOS jobs failed, but it's not related to your changes. We need to fix the CI jobs on macOS. |
Great, thank you. |
Fixes #2003
Fixes #1766
Signed-off-by: Craig Zerouni [email protected]
I was running
rez depends
on a number of packages, and some of them would crash Rez with the message:This happens in
package_search.py
where you have this loop:The problem is that some requirements in that list are not of type
PackageRequest
, but instead are simply typestr
. This happens if in thepackage.py
file of the package being queried there is aprivate_build_requires
configured as a late-binding function. Eg, a package with this this pbr will not crash Rezbut this one will
I initially assumed this was a bug in the way these are handled, except that in the file
resolved_context.py
I see this comment:which leads me to think that requests of type
str
are expected. In which case, the fix is simple, as per this PR.