Page MenuHomeSoftware Heritage

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

Authored by lewo on Mar 10 2020, 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
Branch
functional-eval-revision
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11074
Build 16688: tox-on-jenkinsJenkins
Build 16687: arc lint + arc unit

Event Timeline

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).

swh/loader/package/functional/loader.py
33

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

77

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

  • 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 marked 2 inline comments as done.
  • functional: create a branch named evaluation pointing to the evaluation commit
  • functional: create a branch named evaluation pointing to the evaluation commit

Try to base on functional-loader branch

swh/loader/package/functional/loader.py
100

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.Mar 12 2020, 9:38 AM
lewo added inline comments.
swh/loader/package/functional/loader.py
100

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.

swh/loader/package/functional/loader.py
100

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.

  • package.loader: add extra_branches method
  • functional: create a branch named evaluation pointing to the evaluation commit
lewo marked 2 inline comments as done.Mar 16 2020, 5:04 PM
  • package.loader: add extra_branches method
  • functional: create a branch named evaluation pointing to the evaluation commit
  • package.loader: add extra_branches method
  • functional: create a branch named evaluation pointing to the evaluation commit

Thanks

Looks good to me.

I'll have another look tomorrow morning.

Cheers,

swh/loader/package/functional/loader.py
29

types

def __init__(self, url: str, origin: Optional[str]):
36

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

84

pointer can target a nonexistent revision for a time.

supposed to load

swh/loader/package/loader.py
336

Please, give better name to k and v.

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

This revision now requires changes to proceed.Mar 18 2020, 1:37 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.Mar 18 2020, 3:02 PM
lewo added inline comments.
swh/loader/package/functional/loader.py
29

The origin argument has been removed.

36

It is also used in the get_versions method

swh/loader/package/loader.py
336

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 marked an inline comment as done.
  • functional: create a branch named evaluation pointing to the evaluation commit

Awesome ;)

swh/loader/package/functional/loader.py
36

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

swh/loader/package/loader.py
336

yes, that's quite fine ;)

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