-
Notifications
You must be signed in to change notification settings - Fork 314
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(scancode): No ScanCode license texts in disclosure document #9622
fix(scancode): No ScanCode license texts in disclosure document #9622
Conversation
303e82f
to
7f58b15
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9622 +/- ##
============================================
+ Coverage 68.02% 68.16% +0.14%
- Complexity 1285 1302 +17
============================================
Files 249 249
Lines 8828 8946 +118
Branches 921 953 +32
============================================
+ Hits 6005 6098 +93
- Misses 2433 2457 +24
- Partials 390 391 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7f58b15
to
e01d408
Compare
d24b05e
to
de1b915
Compare
de1b915
to
dd37d32
Compare
Dockerfile
Outdated
@@ -157,6 +157,11 @@ RUN ARCH=$(arch | sed s/aarch64/arm64/) \ | |||
rm requirements.txt; \ | |||
fi | |||
|
|||
# Extract ScanCode license texts to a directory | |||
RUN scancode-license-data --path=/opt/scancode-license-data \ | |||
&& find /opt/scancode-license-data -type f ! -name "*.LICENSE" -exec rm -f {} + \ |
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.
Nit: Please use the more speaking -not
instead of !
(as the latter also has a special meaning in Bash).
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 had a second thought on it: If scancode-license-data is the "API" way to access the license texts, then I don't want to put implementation-specific knowledge into the Dockerfile like what files I expect to be there, and which ones to delete. Because that might change over time, even without notice.
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.
If scancode-license-data is the "API" way to access the license texts
Well, the scancode-license-data
command was originally designed to dump the data for a static website. In that sense, we're just "misusing" it here, and it's by-design that it does more than what we need.
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.
Because that might change over time, even without notice.
True. But to guard against that, I was planning to make a follow-up change that runs ort requirements
as part of the Docker image build, to fail if ORT cannot find any licenses. We already were doing that at some point, but apparently it got dropped during some Docker image build process refactoring.
Also the commit message title should not be the one of the bug getting fixed, but say what's being done:
|
dd37d32
to
24e19d0
Compare
@@ -493,6 +499,9 @@ ENV GEM_HOME=/var/tmp/gem | |||
ENV PATH=$PATH:$RBENV_ROOT/bin:$RBENV_ROOT/shims:$RBENV_ROOT/plugins/ruby-install/bin | |||
COPY --from=ruby --chown=$USER:$USER $RBENV_ROOT $RBENV_ROOT | |||
|
|||
# ScanCode license texts directory | |||
COPY --from=scancode-license-data --chown=$USER:$USER /opt/scancode-license-data /opt/scancode-license-data |
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.
Why did you remove the previous removal of unneeded files? Can we maybe elegantly replace that with only copying /opt/scancode-license-data/*.LICENSE
here?
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.
Ah, I missed your comment as you already resolved / collapsed the conversation. So, still, what do you think about just copying needed files here?
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.
Copying the needed stuff is more complicated than expected, so I went back deleting the files we don't need.
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.
Copying the needed stuff is more complicated than expected
Oh, interesting, mind sharing the details? Why does
COPY --from=scancode-license-data --chown=$USER:$USER /opt/scancode-license-data/*.LICENSE /opt/scancode-license-data
not work?
I went back deleting the files we don't need.
Unfortunately, that now seems to miss deleting the static
directory, or?
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.
Ping @wkl3nk just in case you overlooked this comment.
Dump the ScanCode license texts to directory /opt/scancode-license-data when creating the reporter docker container. This directory is used by ORT as fallback option if the ScanCode license texts cannot be located by the existing heuristic look-up algorithm. See oss-review-toolkit/ort#9622. Signed-off-by: Wolfgang Klenk <[email protected]>
24e19d0
to
d532222
Compare
Dump the ScanCode license texts to directory /opt/scancode-license-data when creating the ORT docker container. Use this directory as fallback option if the ScanCode license texts cannot be located by the existing heuristic algorithm. Fixes oss-review-toolkit#8147. Signed-off-by: Wolfgang Klenk <[email protected]>
d532222
to
e80abee
Compare
Dump the ScanCode license texts to directory /opt/scancode-license-data when creating the reporter docker container. This directory is used by ORT as fallback option if the ScanCode license texts cannot be located by the existing heuristic look-up algorithm. See oss-review-toolkit/ort#9622. Signed-off-by: Wolfgang Klenk <[email protected]>
Dump the ScanCode license texts to directory /opt/scancode-license-data when creating the reporter docker container. This directory is used by ORT as fallback option if the ScanCode license texts cannot be located by the existing heuristic look-up algorithm. See oss-review-toolkit/ort#9622. Signed-off-by: Wolfgang Klenk <[email protected]>
Dump the ScanCode license texts to directory /opt/scancode-license-data when creating the ORT docker container. Use this directory as fallback option if the ScanCode license texts cannot be located by the existing heuristic algorithm.
Fixes #8147.