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
Improve the error message when trying to use a build-scripts
package as a regular requirement
#15366
base: develop2
Are you sure you want to change the base?
Conversation
…e as a regular requirement It currently fails on an assert with no further context provided in the stack trace. There might be a better place to do this check, though. Also, might want to add a check for `PackageType.PYTHON` as well.
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.
Looks good, just a couple of quick things:
- Is there any scenario that this could break to users already leveraging this, even if it could be considered a bug? (I'd also like to check this with the team, labeling as look-into to discuss, it seems it makes sense, but just in case there could be some angle that I'd be missing)
- It would be good to cover this with a test, if you need help or guidance, please ask, we can even contribute the test ourselves
@@ -289,6 +289,8 @@ def transform_downstream(self, pkg_type, require, dep_pkg_type): | |||
downstream_require = Requirement(require.ref, headers=False, libs=False, run=require.run) | |||
elif pkg_type is PackageType.HEADER: | |||
downstream_require = Requirement(require.ref, headers=require.headers, libs=require.libs, run=require.run) | |||
elif pkg_type is PackageType.BUILD_SCRIPTS: | |||
raise ConanError("build-scripts package must be used as a tool_requires(), not a regular requirement") |
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 check is about a build-script depending on a library, not really about it being used as regular requires
. This might be an extra check to do, but it seems we want to do a general check over build-scripts being used only as tool-requires
I just want to mention one case where we are currently using a |
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.
Seeing the feedback, this is too risky, I think it needs some changes:
- Make it a warning instead, not an error
- Most likely better suited in
DepsGraphBuilder._create_new_node
It currently fails on an assert with no further context provided in the stack trace. There might be a better place to do this check, though.
Also, might want to add a check for
PackageType.PYTHON
as well.Changelog: Fix: Improve the error message when trying to use a
build-scripts
package as a regular requirementDocs: Omit
develop
branch, documenting this one.