Page MenuHomeSoftware Heritage

Make the SourceForge lister incremental
AcceptedPublic

Authored by Alphare on Fri, Apr 30, 11:10 PM.

Details

Reviewers
douardda
Group Reviewers
Reviewers
Summary

SourceForge's sitemaps (1 main one + many sharded) give us a "last
modified" date for every subsitemap and project, allowing us to perform
an incremental listing.

We store the subsitemaps' "last modified" dates in the lister state, as
well as those of the empty projects (projects which don't have any VCS
registered), and the rest comes from the already visited origins from
the database.

The tests try to cover the possible cases of a subsitemap that has
changed, one that hasn't, a project that has change, one that hasn't,
and same for an empty project.

Diff Detail

Repository
rDLS Listers
Branch
sourceforge-lister
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21205
Build 32919: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 32918: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D5659 (id=20212)

Rebasing onto 6f8dd5d3f2...

Current branch diff-target is up to date.
Changes applied before test
commit 9aaa7f7795b20aacb62f2b196231629468926d7c
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Fri Apr 30 21:46:29 2021 +0200

    Make the SourceForge lister incremental
    
    SourceForge's sitemaps (1 main one + many sharded) give us a "last
    modified" date for every subsitemap and project, allowing us to perform
    an incremental listing.
    
    We store the subsitemaps' "last modified" dates in the lister state, as
    well as those of the empty projects (projects which don't have any VCS
    registered), and the rest comes from the already visited origins from
    the database.
    
    The tests try to cover the possible cases of a subsitemap that has
    changed, one that hasn't, a project that has change, one that hasn't,
    and same for an empty project.

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

Nice trick.

Jenkins/Phabricator reports most of the new code in listed_origins and _get_pages_for_project is not covered by tests, could you check? (It's sometimes wrong, we don't understand why)

swh/lister/sourceforge/lister.py
57–67

Could you make these comments docstrings, so they show up in the docs?

And don't mean they "have no VCS for us"?

157–158

:D

If it's an issue, wouldn't a dict with ((namespace, project) as key and URLs as values perform better?

295–300

Could you should repeat here what "empty project" means?

316–317

Couldn't this happen if this is a new project, or if the project added a VCS since the last listing?

swh/lister/sourceforge/tests/test_lister.py
253–256

Can you compare the values?

Alphare marked 3 inline comments as done.

Fix incremental testing + incorporate suggestions

Build is green

Patch application report for D5659 (id=20214)

Rebasing onto 6f8dd5d3f2...

Current branch diff-target is up to date.
Changes applied before test
commit 934301cec325b4f0cef1ae0b77a64f36363bb94c
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Fri Apr 30 21:46:29 2021 +0200

    Make the SourceForge lister incremental
    
    SourceForge's sitemaps (1 main one + many sharded) give us a "last
    modified" date for every subsitemap and project, allowing us to perform
    an incremental listing.
    
    We store the subsitemaps' "last modified" dates in the lister state, as
    well as those of the empty projects (projects which don't have any VCS
    registered), and the rest comes from the already visited origins from
    the database.
    
    The tests try to cover the possible cases of a subsitemap that has
    changed, one that hasn't, a project that has change, one that hasn't,
    and same for an empty project.

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

Jenkins/Phabricator reports most of the new code in listed_origins and _get_pages_for_project is not covered by tests, could you check? (It's sometimes wrong, we don't understand why)

That would be because I forgot to add incremental=True to my lister 👼 . I'll see with the next refresh what the coverage looks like.

I've made it so it would break if that happened again.

swh/lister/sourceforge/lister.py
157–158

This is clearly a case of trying to be too clever at 10pm on a Friday, I think a dict will work fine, hehe.

316–317

Yep, it could. I logged a less scary message which would still allow us to debug in case of a mistake.

swh/lister/sourceforge/tests/test_lister.py
253–256

I'm not sure which values you're refering to.

Build is green

Patch application report for D5659 (id=20215)

Rebasing onto 6f8dd5d3f2...

Current branch diff-target is up to date.
Changes applied before test
commit f41424e13ea16b6aa6b295a2a99d00c3b628904f
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Fri Apr 30 21:46:29 2021 +0200

    Make the SourceForge lister incremental
    
    SourceForge's sitemaps (1 main one + many sharded) give us a "last
    modified" date for every subsitemap and project, allowing us to perform
    an incremental listing.
    
    We store the subsitemaps' "last modified" dates in the lister state, as
    well as those of the empty projects (projects which don't have any VCS
    registered), and the rest comes from the already visited origins from
    the database.
    
    The tests try to cover the possible cases of a subsitemap that has
    changed, one that hasn't, a project that has change, one that hasn't,
    and same for an empty project.

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

swh/lister/sourceforge/tests/test_lister.py
253–256

nvm, my comment doesn't make sense. I most have read your code as assert len(stats.pages) == 1

douardda added a subscriber: douardda.

besides the type aliasing statements that I find a bit confusing, LGTM.

swh/lister/sourceforge/lister.py
52

at first I though these better be type annotations (instead of affectations), but I was wrong. Maybe postfix them with a T (eg. ProjectNameT ) to make it clearer these are actually type aliases?

Also, LastModifed is really only a date (with no time)? (edit: looks so, according the tests below)

This revision is now accepted and ready to land.Tue, May 4, 5:17 PM
swh/lister/sourceforge/lister.py
52

Why would these "better be type annotations"? I'm not overly familiar with Python explicit typing, so I'm happy to learn.
I have no problem with postfixing the with a T if that's clearer.
LastModified is indeed a date since SourceForge only provides us with date granularity.