Page MenuHomeSoftware Heritage
Differential D6313

gnu: Respect the pattern docstring about extra state initialization
ClosedPublic
Actions

Authored by ardumont on Sep 21 2021, 10:27 AM.

Details

Reviewers
anlambert
olasd
Group Reviewers
Reviewers
Commits
rDLS5ab6b0040806: gnu: Respect the pattern docstring about state initialization
Summary

Any extra state initialization (outside the scheduler scope) is to happen in the
get_pages method.

Following the discussion in D6310, the gnu lister happens to do that step wrong. So this
fixes it.

Test Plan

tox

Diff Detail

Repository
rDLS Listers
Branch
opam-root-init
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23839
Build 37173: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 37172: arc lint + arc unit

Event Timeline

ardumont created this revision.Sep 21 2021, 10:27 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 21 2021, 10:27 AM
ardumont mentioned this in D6310: opam: Move the state initialization into the get_pages method.Sep 21 2021, 10:29 AM
swh-public-ci added a comment.Sep 21 2021, 10:29 AM

Build is green

Patch application report for D6313 (id=22911)

Could not rebase; Attempt merge onto c803fc2b59...

Updating c803fc2..850805b
Fast-forward
 swh/lister/gnu/lister.py             | 10 +++++--
 swh/lister/opam/lister.py            | 18 ++++++++----
 swh/lister/opam/tests/test_lister.py | 53 ++++++++++++++++++++++++++++++------
 swh/lister/pattern.py                | 13 +++++++++
 4 files changed, 78 insertions(+), 16 deletions(-)
Changes applied before test
commit 850805b1ea6031d5182f02c563bcb7b45ec0bffb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 10:25:31 2021 +0200

    gnu: Respect the pattern docstring about state initialization
    
    Any extra state initialization (outside the scheduler scope) is to happen in the
    get_pages method.

commit 7996d102787950433eaee7ad986c5ddcdf0dcbc0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 15:08:58 2021 +0200

    opam: Allow defining where to actually install the opam_root folder
    
    Related to T3590

commit 0feddfaced6df52b06a145c1e9c905becf61b4d3
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:47:29 2021 +0200

    opam: Make the instance optional and derived from the url
    
    This matches how it's done for all other multi instances listers.
    
    Related to T3590

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/363/ for more details.

Harbormaster completed remote builds in B23839: Diff 22911.Sep 21 2021, 10:29 AM
ardumont requested review of this revision.Sep 21 2021, 10:29 AM
ardumont updated this revision to Diff 22915.Sep 21 2021, 10:37 AM

Rebase

swh-public-ci added a comment.Sep 21 2021, 10:40 AM

Build is green

Patch application report for D6313 (id=22915)

Could not rebase; Attempt merge onto c803fc2b59...

Updating c803fc2..4ff277f
Fast-forward
 swh/lister/gnu/lister.py             | 10 +++++--
 swh/lister/opam/lister.py            | 20 ++++++++------
 swh/lister/opam/tests/test_lister.py | 53 ++++++++++++++++++++++++++++++------
 swh/lister/pattern.py                | 13 +++++++++
 4 files changed, 78 insertions(+), 18 deletions(-)
Changes applied before test
commit 4ff277f42776b32f8adc17513579674126e9fa2d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 10:25:31 2021 +0200

    gnu: Respect the pattern docstring about state initialization
    
    Any extra state initialization (outside the scheduler scope) is to happen in the
    get_pages method.

commit be96c385c24612ee265df42af71e3da09ca99393
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 15:08:58 2021 +0200

    opam: Allow defining where to actually install the opam_root folder
    
    Related to T3590

commit 34880c73a2154a24a6fd3e0f8b707b951f810ec8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:47:29 2021 +0200

    opam: Make the instance optional and derived from the url
    
    This matches how it's done for all other multi instances listers.
    
    Related to T3590

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/367/ for more details.

Harbormaster completed remote builds in B23843: Diff 22915.Sep 21 2021, 10:40 AM
ardumont updated this revision to Diff 22919.Sep 21 2021, 10:49 AM

Rebase

swh-public-ci added a comment.Sep 21 2021, 10:52 AM

Build is green

Patch application report for D6313 (id=22919)

Could not rebase; Attempt merge onto c803fc2b59...

Updating c803fc2..fddba17
Fast-forward
 swh/lister/gnu/lister.py             | 10 +++++--
 swh/lister/opam/lister.py            | 20 +++++++------
 swh/lister/opam/tests/test_lister.py | 54 ++++++++++++++++++++++++++++++------
 3 files changed, 66 insertions(+), 18 deletions(-)
Changes applied before test
commit fddba1794b7e6819c8ea3842f3eaea21d661b9f0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 10:25:31 2021 +0200

    gnu: Respect the pattern docstring about state initialization
    
    Any extra state initialization (outside the scheduler scope) is to happen in the
    get_pages method.

commit 50700fd47379c05b440ad6e97a905ec265a08b8d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 15:08:58 2021 +0200

    opam: Allow defining where to actually install the opam_root folder
    
    Related to T3590

commit 2398ca85c8c4685465945c305a67279a67f7ba55
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:47:29 2021 +0200

    opam: Make the instance optional and derived from the url
    
    This matches how it's done for all other multi instances listers.
    
    Related to T3590

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/371/ for more details.

Harbormaster completed remote builds in B23847: Diff 22919.Sep 21 2021, 10:52 AM
anlambert accepted this revision.Sep 21 2021, 10:55 AM
anlambert added a subscriber: anlambert.

Looks good to me.

This revision is now accepted and ready to land.Sep 21 2021, 10:55 AM
olasd accepted this revision.Sep 21 2021, 10:56 AM
olasd added a subscriber: olasd.

Thank you!

swh/lister/gnu/lister.py
48

isn't that a tad redundant with the previous line?

ardumont updated this revision to Diff 22920.Sep 21 2021, 10:58 AM

Drop unneeded assertion

swh/lister/gnu/lister.py
48

yes, removing.

swh-public-ci added a comment.Sep 21 2021, 11:01 AM

Build is green

Patch application report for D6313 (id=22920)

Could not rebase; Attempt merge onto c803fc2b59...

Updating c803fc2..771ab44
Fast-forward
 swh/lister/gnu/lister.py             |  9 ++++--
 swh/lister/opam/lister.py            | 20 +++++++------
 swh/lister/opam/tests/test_lister.py | 54 ++++++++++++++++++++++++++++++------
 3 files changed, 65 insertions(+), 18 deletions(-)
Changes applied before test
commit 771ab445a3a03d05e1ae12f501e0df1b756aa767
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 10:25:31 2021 +0200

    gnu: Respect the pattern docstring about state initialization
    
    Any extra state initialization (outside the scheduler scope) is to happen in the
    get_pages method.

commit 50700fd47379c05b440ad6e97a905ec265a08b8d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 15:08:58 2021 +0200

    opam: Allow defining where to actually install the opam_root folder
    
    Related to T3590

commit 2398ca85c8c4685465945c305a67279a67f7ba55
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:47:29 2021 +0200

    opam: Make the instance optional and derived from the url
    
    This matches how it's done for all other multi instances listers.
    
    Related to T3590

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/372/ for more details.

Harbormaster completed remote builds in B23848: Diff 22920.Sep 21 2021, 11:01 AM
ardumont updated this revision to Diff 22925.Sep 21 2021, 11:19 AM

Rebase

swh-public-ci added a comment.Sep 21 2021, 11:22 AM

Build is green

Patch application report for D6313 (id=22925)

Could not rebase; Attempt merge onto c803fc2b59...

Updating c803fc2..5ab6b00
Fast-forward
 swh/lister/gnu/lister.py             |  9 ++++--
 swh/lister/opam/lister.py            | 20 +++++++------
 swh/lister/opam/tests/test_lister.py | 54 ++++++++++++++++++++++++++++++------
 3 files changed, 65 insertions(+), 18 deletions(-)
Changes applied before test
commit 5ab6b0040806844762e6ae229c95555564a6b2ea
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 10:25:31 2021 +0200

    gnu: Respect the pattern docstring about state initialization
    
    Any extra state initialization (outside the scheduler scope) is to happen in the
    get_pages method.

commit 332ed8e54309985e35acb1e7023a7a2ca6e9e88c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 15:08:58 2021 +0200

    opam: Allow defining where to actually install the opam_root folder
    
    Related to T3590

commit ff5e86ff489eb1d3c504fe6f0a3392db4d8b6c19
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Sep 20 17:47:29 2021 +0200

    opam: Make the instance optional and derived from the url
    
    This matches how it's done for all other multi instances listers.
    
    Related to T3590

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/376/ for more details.

Harbormaster completed remote builds in B23854: Diff 22925.Sep 21 2021, 11:22 AM
Closed by commit rDLS5ab6b0040806: gnu: Respect the pattern docstring about state initialization (authored by ardumont). · Explain WhySep 21 2021, 11:23 AM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLS5ab6b0040806: gnu: Respect the pattern docstring about state initialization.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
lister/
gnu/
10 lines100%
Loading...

Diff 22911

View Options

swh/lister/gnu/lister.py

Loading...
About · Developer information · Legal