-
Notifications
You must be signed in to change notification settings - Fork 991
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 tools.info.package_id:confs
when pattern did not match any defined conf
#15353
Fix tools.info.package_id:confs
when pattern did not match any defined conf
#15353
Conversation
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.
Some explanatory comments
@@ -481,7 +481,7 @@ def copy_conaninfo_conf(self): | |||
# Reading the list of all the configurations selected by the user to use for the package_id | |||
package_id_confs = self.get("tools.info.package_id:confs", default=[], check_type=list) | |||
for conf_name in package_id_confs: | |||
matching_confs = [c for c in self._values if re.match(conf_name, c)] or [conf_name] | |||
matching_confs = [c for c in self._values if re.match(conf_name, c)] |
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 original idea from @thorsten-klein seems to have been that if no match was found, then the passed conf would be a specific conf id - This is not the case for patterns that just don't match anything, either because they are malformed if they are meant to add built in confs, or because they target user. confs that are not currently defined.
I don't see a need to keep the or branch as proper confs will only match themselves, so removing that bit should not change behaviour in any case, so this not breaking, only exclusively a bugfix
This bit comes from the original pr at #14621
('["user.foo:value", "user.bar:value"]', PKG_ID_USER_2), | ||
('["user.*"]', PKG_ID_USER_3), |
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.
@memsharded this is unrelated to the PR - I initially added this to check that user confs were being handled properly. But note how the generated pkgids are different in these 2 cases, we need to look into that too
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 solved by #15356
|
||
# This used to break the build because `user.*` conf was not valid | ||
tc.run("create lib") | ||
assert "lib/1.0: Package 'da39a3ee5e6b4b0d3255bfef95601890afd80709' created" in tc.out |
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 proper test that was failing 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.
I am also fine if this is released in 2.0.17 as a fix too (it can be in 2.1 too)
Noted, will cherry-pick the squashed commit once this lands in |
Changelog: Fix: Fix
tools.info.package_id:confs
when pattern did not match any defined conf.Docs: Omit