-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
adapt code to new spdx-tools release #3173
adapt code to new spdx-tools release #3173
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.
Here are two more locations that probably need to be adapted:
https://github.com/nexB/scancode-toolkit/blob/8f07fdfb678aaed6e8eef940f6c0241e53f5614a/setup.cfg#L107
https://github.com/nexB/scancode-toolkit/blob/8f07fdfb678aaed6e8eef940f6c0241e53f5614a/setup-mini.cfg#L106
@meretp @nicoweidner I pushed spdx-tools 0.7.0rc0 so you should be able to update your PR accordingly. |
50a1233
to
0efa0ff
Compare
@pombredanne I updated the PR accordingly and all tests pass now, so this is ready for review. |
I updated the |
@pombredanne Gentle ping, is there anything else I can do to get this PR merged? |
We are all good and this is being merged today |
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.
@meretp Thanks! see some review nitpickings for your consideration.
setup-mini.cfg
Outdated
@@ -103,7 +103,7 @@ install_requires = | |||
pymaven_patch >= 0.2.8 | |||
requests >= 2.7.0 | |||
saneyaml >= 0.5.2 | |||
spdx_tools == 0.7.0a3 | |||
spdx_tools >= 0.7.0rc0, ==0.7.* |
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 avoid using upper cap in setup requirements. We pin instead in requirements files.
spdx_tools >= 0.7.0rc0, ==0.7.* | |
spdx_tools >= 0.7.0rc0 |
See https://iscinumpy.dev/post/bound-version-constraints/ and https://caremad.io/posts/2013/07/setup-vs-requirement/ for detailed articles on the topics. ScanCode is both used as an app and as a library. App deps are pinned, library deps are not capped.
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.
Since there is currently a big refactoring in the progress, which is breaking the API after 0.7, we think that it is safer to restrict it here. Otherwise we will run into the same race condition with releasing.
Pinning in requirement files was not sufficient to mitigate the breaking changes between 0.7.0a3
and 0.7.0rc0
.
With the new release, the checksum class has been renamed, the license class has been moved to its own file, and files are now only allowed at document level. Signed-off-by: Meret Behrens <[email protected]>
Signed-off-by: Meret Behrens <[email protected]>
Signed-off-by: Meret Behrens <[email protected]>
0a1774e
to
5c7c4a8
Compare
@pombredanne I included your suggested changes in a fix-up commit to squash this before merging. Two tests fail but that seems to be another issue as the latest commit on develop also has these failing tests. |
No worries, we will be fine now! |
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 test failure is unrelated and fixed in develop. I still prefer a pinned SPDX version for now.
Signed-off-by: Philippe Ombredanne <[email protected]>
b51387d
to
4677510
Compare
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.
Thanks! The code looks good and the failure is fixed in develop.
I can merge as is!
I fixed the requirements to be pinned for SPDX tools for now as I do not like stars.
With the new release, the checksum class has been renamed, the license class has been moved to its own file, and files are now only allowed at document level.
Signed-off-by: Meret Behrens [email protected]
Fixes #3172
Tasks
Run tests locally to check for errors.