Skip to content
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

Conversation

wkl3nk
Copy link
Contributor

@wkl3nk wkl3nk commented Dec 16, 2024

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.

@wkl3nk wkl3nk requested a review from a team as a code owner December 16, 2024 15:56
@wkl3nk wkl3nk force-pushed the wkl3nk/quick-fix-scancode-license-text-location branch from 303e82f to 7f58b15 Compare December 16, 2024 16:00
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.16%. Comparing base (c259ffb) to head (e80abee).
Report is 43 commits behind head on main.

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     
Flag Coverage Δ
funTest-docker 65.14% <ø> (+0.17%) ⬆️
funTest-non-docker 33.28% <ø> (-0.04%) ⬇️
test-ubuntu-24.04 35.91% <ø> (+0.10%) ⬆️
test-windows-2022 35.88% <ø> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wkl3nk wkl3nk force-pushed the wkl3nk/quick-fix-scancode-license-text-location branch from 7f58b15 to e01d408 Compare December 16, 2024 16:13
utils/spdx/src/main/kotlin/Utils.kt Outdated Show resolved Hide resolved
@wkl3nk wkl3nk force-pushed the wkl3nk/quick-fix-scancode-license-text-location branch 2 times, most recently from d24b05e to de1b915 Compare December 17, 2024 08:02
@wkl3nk wkl3nk requested a review from sschuberth December 17, 2024 08:40
@wkl3nk wkl3nk marked this pull request as draft December 18, 2024 09:57
@wkl3nk wkl3nk force-pushed the wkl3nk/quick-fix-scancode-license-text-location branch from de1b915 to dd37d32 Compare December 18, 2024 21:15
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
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 {} + \
Copy link
Member

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).

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

@sschuberth
Copy link
Member

fix(scancode): No ScanCode license texts in disclosure document

Also the commit message title should not be the one of the bug getting fixed, but say what's being done:

fix(scancode): Ensure to find license texts in the Docker image

@wkl3nk wkl3nk force-pushed the wkl3nk/quick-fix-scancode-license-text-location branch from dd37d32 to 24e19d0 Compare December 19, 2024 08:59
@wkl3nk wkl3nk marked this pull request as ready for review December 19, 2024 09:03
@wkl3nk wkl3nk requested a review from sschuberth December 19, 2024 09:04
@@ -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
Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

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.

Dockerfile Outdated Show resolved Hide resolved
wkl3nk added a commit to boschglobal/ort-server that referenced this pull request Dec 19, 2024
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]>
@wkl3nk wkl3nk force-pushed the wkl3nk/quick-fix-scancode-license-text-location branch from 24e19d0 to d532222 Compare December 19, 2024 15:22
@wkl3nk wkl3nk requested a review from sschuberth December 19, 2024 15:43
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]>
@wkl3nk wkl3nk force-pushed the wkl3nk/quick-fix-scancode-license-text-location branch from d532222 to e80abee Compare December 20, 2024 08:20
@sschuberth sschuberth merged commit 958c08c into oss-review-toolkit:main Dec 20, 2024
25 checks passed
wkl3nk added a commit to boschglobal/ort-server that referenced this pull request Dec 20, 2024
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]>
wkl3nk added a commit to boschglobal/ort-server that referenced this pull request Dec 20, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot find ScanCode license texts in Docker image
2 participants