Differential D8773

nixguix: Deal with edge case url with version instead of extension
ClosedPublic
Actions

Authored by ardumont on Oct 25 2022, 5:29 PM.

Details

Reviewers

anlambert

Group Reviewers

Reviewers

Maniphest Tasks

T3781: Replace the Nixguix loader with a lister

Commits

rDLS026fea21da49: nixguix: Deal with edge case url with version instead of extension

Summary

Prior to this, some urls were detected as file because their version name were wrongly
detected as extension, hence not matching tarball extensions.

Related to T3781

(Note: Connectivity issues on my side)

Diff Detail

Repository

rDLS Listers

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 32584
Build 51041: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 51040: arc lint + arc unit

Event Timeline

ardumont created this revision.Oct 25 2022, 5:29 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptOct 25 2022, 5:29 PM

Build is green

Patch application report for D8773 (id=31631)

Rebasing onto 8355fee25f...

First, rewinding head to replay your work on top of it...
Applying: nixguix: Deal with edge case url with version instead of extension

Changes applied before test

commit e165d09ecd16014e00a4d05d3d2a0e99ac725a9b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Oct 25 17:04:29 2022 +0200

    nixguix: Deal with edge case url with version instead of extension
    
    Prior to this, some urls were detected as file because their version name were wrongly
    detected as extension, hence not matching tarball extensions.
    
    Related to T3781

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

Harbormaster completed remote builds in B32584: Diff 31631.Oct 25 2022, 5:34 PM

ardumont requested review of this revision.Oct 25 2022, 5:34 PM

ardumont added a child revision: D8774: nixguix: Use content-disposition from http head request if provided.Oct 25 2022, 5:50 PM

ardumont added inline comments.Oct 25 2022, 6:03 PM

swh/lister/nixguix/lister.py
140	probably too open a regexp...
140	opened*

Improve the regexp version detection to be more restrictive.

I think detecting versions is a lost cause. Here is what I had to do for PyPI: https://forge.softwareheritage.org/source/swh-storage/browse/master/swh/storage/migrate_extrinsic_metadata.py$234-281

with lots of annoying examples here: https://forge.softwareheritage.org/source/swh-storage/browse/master/swh/storage/tests/migrate_extrinsic_metadata/test_pypi.py$62-146

plus, Nix and Guix already have the package name and version in their metadata, can't we ask them to provide this data to us?

Build is green

Patch application report for D8773 (id=31635)

Rebasing onto 8355fee25f...

First, rewinding head to replay your work on top of it...
Applying: nixguix: Deal with edge case url with version instead of extension

Changes applied before test

commit 4cc46f34fb4a929afc67d560fd10765c94ec1bbd
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Oct 25 17:04:29 2022 +0200

    nixguix: Deal with edge case url with version instead of extension
    
    Prior to this, some urls were detected as file because their version name were wrongly
    detected as extension, hence not matching tarball extensions.
    
    Related to T3781

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

Harbormaster completed remote builds in B32588: Diff 31635.Oct 25 2022, 6:21 PM

In D8773#228150, @vlorentz wrote:

plus, Nix and Guix already have the package name and version in their metadata, can't we ask them to provide this data to us?

er actually, it's the file type we need. But couldn't they provide this as well?

I think detecting versions is a lost cause. Here is what I had to do for PyPI:
https://forge.softwareheritage.org/source/swh-storage/browse/master/swh/storage/migrate_extrinsic_metadata.py$234-281

With this and the next diff, the listing will be complete (in guix at least). I've some
local analysis that i've yet to push again (after some more docker runs) tomorrow.

with lots of annoying examples here:

https://forge.softwareheritage.org/source/swh-storage/browse/master/swh/storage/tests/migrate_extrinsic_metadata/test_pypi.py$62-146

Yes, it's complicated and annoying! I'll look in more details tomorrow to your links.

plus, Nix and Guix already have the package name and version in their metadata, can't

we ask them to provide this data to us? er actually, it's the file type we need. But
couldn't they provide this as well?

I'm pretty sure zimoun would not mind adding those indeed. But that will be enough for
Guix but won't be enough for nixpkgs (I don't really like half fixes).

For the nixpkgs part, I'm not sure they will react, as far as the nixpkgs issues I've
opened [1] [2] goes, nothing bulged in 20 days or so ¯\_(ツ)_/¯.

[1] T4608 (upstream issue linked from it)

[2] T4609 (upstream issue linked from it)

ardumont mentioned this in T3781: Replace the Nixguix loader with a lister.Oct 25 2022, 6:53 PM

I am not a big fan either of adding more tarball detection heuristics but after quickly hacking on the code, it seems this is the only way to handle these edge case URLS (plus there is some cases when even analyzing HTTP headers cannot help to detect if the URL targets a tarball P1512). So let's land this and move on in deploying and testing that lister on staging.

This revision is now accepted and ready to land.Oct 26 2022, 11:34 AM

Rebase (commit id mismatch locally and in the diff, don't recall what changed)
-> must be the commit message.

Build is green

Patch application report for D8773 (id=31638)

Rebasing onto 8355fee25f...

Current branch diff-target is up to date.

Changes applied before test

commit 026fea21da49b30fda9c5adbb24c6d7a8c24c8df
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Oct 25 17:04:29 2022 +0200

    nixguix: Deal with edge case url with version instead of extension
    
    Prior to this, some urls were detected as file because their version name were wrongly
    detected as extension, hence not matching tarball extensions.
    
    Related to T3781

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

Harbormaster completed remote builds in B32591: Diff 31638.Oct 26 2022, 11:49 AM

Closed by commit rDLS026fea21da49: nixguix: Deal with edge case url with version instead of extension (authored by ardumont). · Explain WhyOct 26 2022, 12:00 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDLS026fea21da49: nixguix: Deal with edge case url with version instead of extension.