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.

Sign up for GitHub

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

Jump to bottom

Ignore large data files and bump scancode-toolkit #1508

Merged
merged 13 commits into from
Jan 21, 2025
Merged

Ignore large data files and bump scancode-toolkit #1508

merged 13 commits into from
Jan 21, 2025

Conversation

AyanSinhaMahapatra
Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra commented Jan 6, 2025 •
edited
Loading

This PR:

@AyanSinhaMahapatra
Ignore scanning large data files
fc51df8 
Ignore scanning large data files which are larger than 1 MB
to avoid crashing scans on memory spikes.
Also rollback #1504

Reference: aboutcode-org/scancode-toolkit#3711
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
@AyanSinhaMahapatra
Bump scancode-toolkit to version v32.3.1
6d6a6a1 
Also remove platform constraints from rust-inspector and
go-inspector.

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
pombredanne
pombredanne approved these changes Jan 6, 2025
View reviewed changes
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@pombredanne
Copy link
Member

we will need some tests later.

tdruez
tdruez requested changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@tdruez tdruez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hardcoded values, SKIP_DATA_FILE_SIZE should be a SCIO setting, similar to SCANCODEIO_SCAN_FILE_TIMEOUT, off by default. Also, I don't think the SKIP_DATA_FILE_EXTENSIONS is relevant.

I would suggest adding a SCANCODEIO_SCAN_MAX_FILE_SIZE, default to None. This is the least intrusive way and will stay useful in the future.

tdruez
tdruez reviewed Jan 6, 2025
View reviewed changes
scanpipe/pipes/scancode.py Outdated Show resolved Hide resolved
@AyanSinhaMahapatra
Copy link
Member Author

@tdruez I've added a SCANCODEIO_SCAN_MAX_FILE_SIZE as suggested by you since this seemed less problematic, this is ready for review.

tdruez
tdruez requested changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@tdruez tdruez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, ready to merge once the couple small issues are addressed.

scanpipe/pipes/flag.py Outdated Show resolved Hide resolved
scanpipe/tests/pipes/test_scancode.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
pombredanne
pombredanne reviewed Jan 20, 2025
View reviewed changes
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we should have a scancod-config entry for this parameter, otherwise this requires changing the installation settings. This could be a follow up issue.

@AyanSinhaMahapatra
Bump scancode-toolkit to v32.3.2
eb7157e 
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
tdruez
tdruez requested changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@tdruez tdruez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AyanSinhaMahapatra The code looks good.
We are missing a changelog entry and to update the https://scancodeio.readthedocs.io/en/latest/project-configuration.html with the new setting.

@AyanSinhaMahapatra
Copy link
Member Author

Thanks for the reviews @tdruez and @pombredanne, all addressed and ready now!

@tdruez tdruez merged commit 1275d07 into main Jan 21, 2025
9 checks passed
@tdruez tdruez deleted the ignore-large-json branch January 21, 2025 12:04
@tdruez
Copy link
Contributor

tdruez commented Jan 21, 2025

@AyanSinhaMahapatra Looks great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pombredanne pombredanne pombredanne approved these changes

@tdruez tdruez Awaiting requested review from tdruez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AyanSinhaMahapatra @pombredanne @tdruez