Page MenuHomeSoftware Heritage

functional.loader: add a pointer to the eval revision
ClosedPublic

Authored by lewo on Tue, Mar 10, 6:48 PM.

Details

Summary

Create a branch in the snapshot pointing to the revision of the commit used to create the sources.json file.

Depends on D2792

Related to T1991

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lewo created this revision.Tue, Mar 10, 6:48 PM
ardumont edited the summary of this revision. (Show Details)Wed, Mar 11, 8:00 AM
ardumont added a parent revision: D2792: Functional Package Loader.

For stacked diff (within the same repo), you need to add the 'Depends on <DIFF-ID>' stanza in the description.
This will allow the jenkins build to pass (if the build diffs you depend on pass though ;)

For D2792, the build is red for now (you need to update the cli test which adds the functional loader in the help message).

ardumont added inline comments.Wed, Mar 11, 10:05 AM
swh/loader/package/functional/loader.py
34 ↗(On Diff #9968)

Maybe mention it's the git revision of nixpkgs/guix.

75 ↗(On Diff #9968)

Please, add the hook branches's specific docstring use here.

lewo updated this revision to Diff 9999.Wed, Mar 11, 3:19 PM
  • package.loader: ignore non tarball source
  • package.loader: add origin argument
  • Add the functional loader
  • cli: add the functional loader in the cli tests
  • package.loader: add hook_branches method
  • functional: create a branch named evaluation pointing to the evaluation commit
lewo updated this revision to Diff 10000.Wed, Mar 11, 3:31 PM
lewo marked 2 inline comments as done.
  • functional: create a branch named evaluation pointing to the evaluation commit
lewo updated this revision to Diff 10001.Wed, Mar 11, 3:32 PM
  • functional: create a branch named evaluation pointing to the evaluation commit
lewo updated this revision to Diff 10002.Wed, Mar 11, 3:46 PM

Try to base on functional-loader branch

ardumont added inline comments.Wed, Mar 11, 5:45 PM
swh/loader/package/functional/loader.py
100 ↗(On Diff #10002)

Instead of mutating the reference here, make that return additional branches the main loader needs to append to its current list.

The contract becomes clearer for that method.
It simply provides the list of extra branches.
That's up to the package loader's basis to deal with those branches. And that's then shared between all loaders.

Maybe even rename hook_branches to extra_branches.

lewo planned changes to this revision.Thu, Mar 12, 9:38 AM
lewo added inline comments.
swh/loader/package/functional/loader.py
100 ↗(On Diff #10002)

I initially wanted to create an extra_branches method, but it is difficult to know what to do if an extra branch overrides an existing one: since branches is a dict, we can not just append elements.
By providing current branches to the method, the overriding logic can be implemented in the subclass itself. In the functional loader, I want to raise an exception in case of overlap, but some other loaders could just want to override existing entries.

I don't have a strong opinion on this. If you prefer, I could create the extra_branches method and raise an exception in the load method in case of overlap.

ardumont added inline comments.Fri, Mar 13, 3:49 PM
swh/loader/package/functional/loader.py
100 ↗(On Diff #10002)

I don't have a strong opinion on this. If you prefer, I could create the extra_branches method and raise an exception in the load method in case of overlap.

Yes, i prefer this.
When we'll have another need, we'll adapt then.

lewo updated this revision to Diff 10087.Mon, Mar 16, 5:03 PM
  • package.loader: add extra_branches method
  • functional: create a branch named evaluation pointing to the evaluation commit
lewo marked 2 inline comments as done.Mon, Mar 16, 5:04 PM
lewo updated this revision to Diff 10090.Mon, Mar 16, 5:25 PM
  • package.loader: add extra_branches method
  • functional: create a branch named evaluation pointing to the evaluation commit
lewo updated this revision to Diff 10092.Mon, Mar 16, 7:10 PM
  • package.loader: add extra_branches method
  • functional: create a branch named evaluation pointing to the evaluation commit
ardumont edited the summary of this revision. (Show Details)Mon, Mar 16, 9:49 PM

Thanks

Looks good to me.

I'll have another look tomorrow morning.

Cheers,

ardumont added inline comments.Wed, Mar 18, 10:35 AM
swh/loader/package/functional/loader.py
39 ↗(On Diff #10092)

types

def __init__(self, url: str, origin: Optional[str]):
47 ↗(On Diff #10092)

nitpick, keep only what you need, that is drop self.sources here :)

82 ↗(On Diff #10092)

pointer can target a nonexistent revision for a time.

supposed to load

swh/loader/package/loader.py
329

Please, give better name to k and v.

ardumont requested changes to this revision.Wed, Mar 18, 1:37 PM

for the sake of my comments and to not forget ;)

This revision now requires changes to proceed.Wed, Mar 18, 1:37 PM
lewo updated this revision to Diff 10126.Wed, Mar 18, 3:01 PM
lewo marked 3 inline comments as done.
  • package.loader: add extra_branches method
  • functional: create a branch named evaluation pointing to the evaluation commit
lewo marked an inline comment as done.Wed, Mar 18, 3:02 PM
lewo added inline comments.
swh/loader/package/functional/loader.py
39 ↗(On Diff #10092)

The origin argument has been removed.

47 ↗(On Diff #10092)

It is also used in the get_versions method

swh/loader/package/loader.py
329

Hm, mypy doesn't allow to change the type of a varaible in a scope. So, I add to rename k to name (and not to branch_name which is already used).

lewo updated this revision to Diff 10128.Wed, Mar 18, 3:07 PM
lewo marked an inline comment as done.
  • functional: create a branch named evaluation pointing to the evaluation commit
ardumont accepted this revision.Wed, Mar 18, 4:53 PM

Awesome ;)

swh/loader/package/functional/loader.py
47 ↗(On Diff #10092)

heh right, i misread (it was folded)
so nvm, my bad ;)

swh/loader/package/loader.py
329

yes, that's quite fine ;)

This revision is now accepted and ready to land.Wed, Mar 18, 4:53 PM