Details

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.Mar 11 2020, 10:05 AM

swh/loader/package/functional/loader.py
43	Maybe mention it's the git revision of nixpkgs/guix.
74	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

Build is green
See https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/351/ for more details.

Harbormaster completed remote builds in B11071: Diff 9999.Mar 11 2020, 3:20 PM

functional: create a branch named evaluation pointing to the evaluation commit

Build is green
See https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/352/ for more details.

Harbormaster completed remote builds in B11072: Diff 10000.Mar 11 2020, 3:32 PM

functional: create a branch named evaluation pointing to the evaluation commit

Build is green
See https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/353/ for more details.

Harbormaster completed remote builds in B11073: Diff 10001.Mar 11 2020, 3:34 PM

Try to base on functional-loader branch

Build is green
See https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/354/ for more details.

Harbormaster completed remote builds in B11074: Diff 10002.Mar 11 2020, 3:48 PM

ardumont added inline comments.Mar 11 2020, 5:45 PM

swh/loader/package/functional/loader.py
96	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
96	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.Mar 13 2020, 3:49 PM

swh/loader/package/functional/loader.py
96	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

Harbormaster failed remote builds in B11156: Diff 10087!Mar 16 2020, 5:03 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/360/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/360/console

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

Build is green
See https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/362/ for more details.

Harbormaster completed remote builds in B11159: Diff 10090.Mar 16 2020, 5:27 PM

package.loader: add extra_branches method
functional: create a branch named evaluation pointing to the evaluation commit

Build is green
See https://jenkins.softwareheritage.org/job/DLDBASE/job/tox/364/ for more details.

Harbormaster completed remote builds in B11161: Diff 10092.Mar 16 2020, 7:11 PM

ardumont edited the summary of this revision. (Show Details)Mar 16 2020, 9:49 PM

Thanks

Looks good to me.

I'll have another look tomorrow morning.

Cheers,

ardumont added inline comments.Mar 18 2020, 10:35 AM

swh/loader/package/functional/loader.py
38–39	types def __init__(self, url: str, origin: Optional[str]):
46	nitpick, keep only what you need, that is drop self.sources here :)
81	pointer can `target` a `nonexistent` revision for a time. supposed `to` load
swh/loader/package/loader.py
328	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

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
38–39	The origin argument has been removed.
46	It is also used in the `get_versions` method
swh/loader/package/loader.py
328	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).