Page MenuHomeSoftware Heritage

opam: Move the state initialization into the get_pages method
ClosedPublic

Authored by ardumont on Sep 20 2021, 5:44 PM.

Details

Summary

We should avoid side-effects in the constructor as much as possible. That avoids
surprising behavior at object instantiation time. The state if needed must be
initialized into the swh.lister.pattern.Lister.get_pages method, as preconized in the
class docstring.

This also fixes the current test that actually bootstrap a real opam local "clone" in
/tmp.

Test Plan

tox (happy)

Diff Detail

Repository
rDLS Listers
Branch
opam-root-init
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 23844
Build 37183: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 37182: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D6310 (id=22902)

Rebasing onto c803fc2b59...

Current branch diff-target is up to date.
Changes applied before test
commit 05ad61681276c69a63b871e977059871f705a9a7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:41:19 2021 +0200

    opam: Separate side-effects call from the constructor
    
    We should avoid side-effects in the constructor as much as possible. That avoids
    surprising behavior at object instantiation time. The state if needed should be now
    built within the `swh.lister.patter.Lister.prepare` method.
    
    This also fixes the current test that actually bootstrap a real opam local "clone".

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

Build is green

Patch application report for D6310 (id=22906)

Rebasing onto c803fc2b59...

Current branch diff-target is up to date.
Changes applied before test
commit 2d0099bf7206347298e38603130445e79a9bf1dd
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:41:19 2021 +0200

    opam: Separate side-effects call from the constructor
    
    We should avoid side-effects in the constructor as much as possible. That avoids
    surprising behavior at object instantiation time. The state if needed should be now
    built within the `swh.lister.patter.Lister.prepare` method.
    
    This also fixes the current test that actually bootstrap a real opam local "clone".
    
    Related to T3590

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

olasd requested changes to this revision.Sep 20 2021, 6:16 PM
olasd added a subscriber: olasd.

If we want to add this, we should also add it to test_pattern.py tests (which check that the scaffolding actually runs as expected). The documentation of the main pattern class also needs to be updated to add a reference to the new method.

The initial intent in the pattern class was to have any such initialization steps run at the beginning of the main loop, which is the get_pages method. Maybe an explicit prepare method makes sense though.

We should also review other loaders for side effects in the __init__ method. I can at least see the gnu lister pull the tree.json.gz there instead of in get_pages.

This revision now requires changes to proceed.Sep 20 2021, 6:16 PM

If we want to add this, we should also add it to test_pattern.py tests (which check that the scaffolding actually runs as expected). The documentation of the main pattern class also needs to be updated to add a reference to the new method.

Rereading the docstring in question, i'm sold on using the get_pages for now.
(I don't want to dig into the extra audit right now).
I've opened D6313 to align correctly the gnu lister.

I'll readapt this diff the same way, using the get_pages method to do the right thing.

The initial intent in the pattern class was to have any such initialization steps run at the beginning of the main loop, which is the get_pages method. Maybe an explicit prepare method makes sense though.

sold.

We should also review other loaders for side effects in the init method. I can at least see the gnu lister pull the tree.json.gz there instead of in get_pages.

yes, that part i don't want to dig into immediately.

ardumont retitled this revision from opam: Separate side-effects call from the constructor to opam: Move the state initialization into the get_pages method.Sep 21 2021, 10:35 AM
ardumont edited the summary of this revision. (Show Details)
ardumont edited the summary of this revision. (Show Details)

Rework implementation according to suggestion.

Drop the extra prepare method and move the state initialization into the get_pages method.

Build is green

Patch application report for D6310 (id=22912)

Rebasing onto c803fc2b59...

Current branch diff-target is up to date.
Changes applied before test
commit 0af407999ab4932e4dccf51897e0402d748a3f03
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:41:19 2021 +0200

    opam: Move the state initialization into the get_pages method
    
    We should avoid side-effects in the constructor as much as possible. That avoids
    surprising behavior at object instantiation time. The state if needed must be
    initialized into the `swh.lister.pattern.Lister.get_pages` method, as preconized in the
    class docstring.
    
    This also fixes the current test that actually bootstrap a real opam local "clone" in
    /tmp.
    
    Related to T3590

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

Actually drop the now "spurious" prepare method, we are using get_pages now.

Build is green

Patch application report for D6310 (id=22916)

Rebasing onto c803fc2b59...

Current branch diff-target is up to date.
Changes applied before test
commit c160376b17175bd8fa73adddf2e14b8dcfad947b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:41:19 2021 +0200

    opam: Move the state initialization into the get_pages method
    
    We should avoid side-effects in the constructor as much as possible. That avoids
    surprising behavior at object instantiation time. The state if needed must be
    initialized into the `swh.lister.pattern.Lister.get_pages` method, as preconized in the
    class docstring.
    
    This also fixes the current test that actually bootstrap a real opam local "clone" in
    /tmp.
    
    Related to T3590

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

This revision is now accepted and ready to land.Sep 21 2021, 11:03 AM
douardda added inline comments.
swh/lister/opam/lister.py
54

is the ws in get_ pages on purpose?

swh/lister/opam/lister.py
54

nope, fixed, thanks.

Build is green

Patch application report for D6310 (id=22922)

Rebasing onto c803fc2b59...

Current branch diff-target is up to date.
Changes applied before test
commit b69b0b7fd6745cd579acf2e5544c3810217bb542
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:41:19 2021 +0200

    opam: Move the state initialization into the get_pages method
    
    We should avoid side-effects in the constructor as much as possible. That avoids
    surprising behavior at object instantiation time. The state if needed must be
    initialized into the `swh.lister.pattern.Lister.get_pages` method, as preconized in the
    class docstring.
    
    This also fixes the current test that actually bootstrap a real opam local "clone" in
    /tmp.
    
    Related to T3590

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