-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add buildAllDep option && Fix package verstion estimation for ParseSpecCmsswdist. #11132
Add buildAllDep option && Fix package verstion estimation for ParseSpecCmsswdist. #11132
Conversation
Jenkins results:
|
79b3da5
to
e7323bc
Compare
Thanks for your review @vkuznet I think I have addressed your comments. Could you please take another look? |
Jenkins results:
|
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.
Todor, please find some comments along the code for your consideration.
logging.info("%s: specsAll: \n%s", specfile, pformat(specsAll)) | ||
logging.info("%s: sepcsPy: \n%s", specfile, pformat(specsPy)) | ||
if buildAllDep: | ||
for spec in specsAll: |
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.
How about replacing these 3 lines by:
DEPS_SPEC.extend(spec)
and then at the very end of this script, filtering to unique dependencies only (list(set(DEPS_SPEC)))?
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's a valid point. I was just trying to make the intervention as little as possible. But yes lets do it the right way. So I did suggest even shorter path here. Please take a look.
…ecCmsswdist. Fix variable type transition. change DEPS_SPEC type to set && Switch to log level info.
e7323bc
to
d3d618a
Compare
Jenkins results:
|
Jenkins results:
|
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 would get rid of the global variable here, but given that it's only a bin script and you have likely tested it, let's get it merged. Thanks for making those changes.
Fixes #11131
Status
READY
Description
With the current PR we are adding the following options to the
ParseSpecCmsswdist.py
script--all
to be able to parse all spec files not only python relatedAnd we fix version string parsing for non-python packages
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
None
External dependencies / deployment changes
None