Page MenuHomeSoftware Heritage
Differential D8619

nixguix: Improve tarball detection
ClosedPublic
Actions

Authored by ardumont on Oct 4 2022, 8:44 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T3781: Replace the Nixguix loader with a lister
Commits
rDLS2fbd66778f32: nixguix: Improve tarball detection
Summary

Current runs demos that most content files are still tarballs [1]

Without this, some tarballs hidden within query parameters are not detected. This does
some extra effort to detect those to avoid sending noise to loaders.

Related to T3781

[1] Related to P1475

Diff Detail

Repository
rDLS Listers
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 32090
Build 50243: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 50242: arc lint + arc unit

Unit TestsFailed
View All

TimeTest
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.lister.nixguix.tests.test_lister::test_is_tarball_not_so_simple[artifact.php?url=one.zip&foo=bar]
urls = ['https://example.org/a', 'https://example.org/r', 'https://example.org/t', 'https://example.org/i', 'https://example.org/f', 'https://example.org/a', ...] request = None
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.lister.nixguix.tests.test_lister::test_is_tarball_not_so_simple[count.php?f=one.gzip&foo=bar]
urls = ['https://example.org/c', 'https://example.org/o', 'https://example.org/u', 'https://example.org/n', 'https://example.org/t', 'https://example.org/.', ...] request = None
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.lister.nixguix.tests.test_lister::test_is_tarball_not_so_simple[download.php?file=one.tar.gz&foo=bar]
urls = ['https://example.org/d', 'https://example.org/o', 'https://example.org/w', 'https://example.org/n', 'https://example.org/l', 'https://example.org/o', ...] request = None
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.lister.nixguix.tests.test_lister::test_is_tarball_not_so_simple[files?name=one.tbz&foo=bar]
urls = ['https://example.org/f', 'https://example.org/i', 'https://example.org/l', 'https://example.org/e', 'https://example.org/s', 'https://example.org/?', ...] request = None
2 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.lister.arch.lister::swh.lister.arch.lister.size_to_bytes
View Full Test Results (4 Failed · 324 Passed)

Event Timeline

ardumont created this revision.Oct 4 2022, 8:44 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptOct 4 2022, 8:44 PM
swh-public-ci added a comment.Oct 4 2022, 8:50 PM

Build has FAILED

Patch application report for D8619 (id=31121)

Rebasing onto 944d4b5b60...

Current branch diff-target is up to date.
Changes applied before test
commit ab2cf62a1008b2dd73c647f746d6e81e3ea93631
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Oct 4 20:42:20 2022 +0200

    nixguix: Better detection of urls targeting tarballs vs files
    
    Related to T3781

Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/750/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/750/console

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 4 2022, 8:50 PM
Harbormaster failed remote builds in B32090: Diff 31121!
ardumont updated this revision to Diff 31122.Oct 4 2022, 9:01 PM

fix tests

ardumont retitled this revision from nixguix: Better detection of urls targeting tarballs vs files to nixguix: Improve tarball detection.Oct 4 2022, 9:01 PM
ardumont edited the summary of this revision. (Show Details)
swh-public-ci added a comment.Oct 4 2022, 9:06 PM

Build is green

Patch application report for D8619 (id=31122)

Rebasing onto 944d4b5b60...

Current branch diff-target is up to date.
Changes applied before test
commit c4c6df39e142648e066887f0ce6289974c99417d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Oct 4 20:42:20 2022 +0200

    nixguix: Improve tarball detection
    
    Without this, some tarballs hidden within query parameters are not detected. This does
    some extra effort to detect those to avoid sending noise to loaders.
    
    Related to T3781

See https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/751/ for more details.

Harbormaster completed remote builds in B32091: Diff 31122.Oct 4 2022, 9:06 PM
ardumont requested review of this revision.Oct 4 2022, 9:06 PM
vlorentz added a subscriber: vlorentz.Oct 4 2022, 11:14 PM

create a new exception class instead of IndexError, it's unclear that the rest of the function is not actually meant to raise that exception.

vlorentz requested changes to this revision.Oct 4 2022, 11:15 PM
This revision now requires changes to proceed.Oct 4 2022, 11:15 PM
ardumont added a comment.Oct 4 2022, 11:29 PM

create a new exception class instead of IndexError, it's unclear that the rest of the function is not actually meant to raise that exception.

I can tell you it's not supposed to raise outside of this function.
Otherwise, i would have documented it in the main docstring of the function.

ardumont added a child revision: D8620: nixguix: Improve git origins detection.Oct 4 2022, 11:32 PM
ardumont mentioned this in T3781: Replace the Nixguix loader with a lister.Oct 5 2022, 10:06 AM
ardumont updated this revision to Diff 31126.Oct 5 2022, 10:10 AM

Adapt according to suggestion

swh-public-ci added a comment.Oct 5 2022, 10:15 AM

Build is green

Patch application report for D8619 (id=31126)

Rebasing onto 944d4b5b60...

Current branch diff-target is up to date.
Changes applied before test
commit 2fbd66778f3286c77149796339426aeccd11c1d0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Oct 4 20:42:20 2022 +0200

    nixguix: Improve tarball detection
    
    Without this, some tarballs hidden within query parameters are not detected. This does
    some extra effort to detect those to avoid sending noise to loaders.
    
    Related to T3781

See https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/753/ for more details.

Harbormaster completed remote builds in B32095: Diff 31126.Oct 5 2022, 10:15 AM
ardumont added a comment.Oct 5 2022, 10:22 AM

create a new exception class instead of IndexError, it's unclear that the rest of the function is not actually meant to raise that exception.

I can tell you it's not supposed to raise outside of this function.
Otherwise, i would have documented it in the main docstring of the function.

Adapted as requested nonetheless.

anlambert added a subscriber: anlambert.Oct 5 2022, 10:47 AM
anlambert added inline comments.
swh/lister/nixguix/lister.py
151–164

You should check values of all query parameters I think as you cannot guess their names.

You can also make that code shorter the following way:

return any(path.endswith(tuple(TARBALL_EXTENSIONS) for path in (urlparsed.path,) + tuple(parse_qsl(urlparsed.query).keys())))
ardumont added inline comments.Oct 5 2022, 10:56 AM
swh/lister/nixguix/lister.py
151–164

ok, looks like a better approach.

I'm gonna land as is for now.

And do another diff to simplify all this (i have another diff already stacked on this and another incoming diff and I'd like to avoid having to spend my time rebasing stuff ;)

vlorentz accepted this revision.Oct 5 2022, 11:07 AM
This revision is now accepted and ready to land.Oct 5 2022, 11:07 AM
Closed by commit rDLS2fbd66778f32: nixguix: Improve tarball detection (authored by ardumont). · Explain WhyOct 5 2022, 11:11 AM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLS2fbd66778f32: nixguix: Improve tarball detection.

Revision Contents
Changeset List

PathSize
swh/
lister/
nixguix/
20 lines
tests/
19 lines

Diff 31121

View Options

swh/lister/nixguix/lister.py

Loading...
View Options

swh/lister/nixguix/tests/test_lister.py

Loading...
About · Developer information · Legal