Page MenuHomeSoftware Heritage
Differential D6306

opam: Allow defining where to actually install the opam_root folder
ClosedPublic
Actions

Authored by ardumont on Sep 20 2021, 3:10 PM.

Details

Reviewers
douardda
Group Reviewers
System administrators
Reviewers
Maniphest Tasks
T3590: opam loader: Ensure required opam state is shared amongst ingestion/listing runs
Commits
rDLS332ed8e54309: opam: Allow defining where to actually install the opam_root folder
Summary

Related to D6305 (for the production part)
Related to T3590
Depends on D6312

Test Plan

tox

Diff Detail

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

Event Timeline

ardumont created this revision.Sep 20 2021, 3:10 PM
ardumont mentioned this in D6305: opam: Install and maintain up-to-date shared opam root directories.Sep 20 2021, 3:11 PM
swh-public-ci added a comment.Sep 20 2021, 3:13 PM

Build is green

Patch application report for D6306 (id=22895)

Rebasing onto c803fc2b59...

Current branch diff-target is up to date.
Changes applied before test
commit ca5a25ec0a16c375cd5c13be77e125eb6311c4a7
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

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

Harbormaster completed remote builds in B23821: Diff 22895.Sep 20 2021, 3:13 PM
ardumont requested review of this revision.Sep 20 2021, 3:13 PM
douardda accepted this revision.Sep 20 2021, 4:46 PM
douardda added a subscriber: douardda.

LGTM, but how is the new opam_root option expected to be set (in production I mean)?

This revision is now accepted and ready to land.Sep 20 2021, 4:46 PM
ardumont added a comment.EditedSep 20 2021, 4:57 PM

LGTM, but how is the new opam_root option expected to be set (in production I mean)?

It's not expected to be set. Currently, we could not. Our current way of instantiating
the listers would make this break (because other listers which share the configuration
would not know what to do about that opam_root key).

So, it's expected to use that new fallback value.

Since I opened that diff, I'm inclined to make that opam root folder configuration also
depend on the instance_url as well.

Those opam lister/loader are designed to be multi instances. Even though, we have only
one instance "for now". If so, that means, this should be computed within the
constructor.

Something like:

self.opamroot = str(opam_root / self.instance)

(And then adapt the other puppet diff).

ardumont updated this revision to Diff 22905.Sep 20 2021, 5:54 PM
  • Adapt to be multi instance aware
  • Rebase on top of other diffs
ardumont edited the summary of this revision. (Show Details)Sep 20 2021, 5:57 PM
ardumont added a parent revision: D6312: opam: Make the instance optional and derived from the url.
swh-public-ci added a comment.Sep 20 2021, 5:58 PM

Build is green

Patch application report for D6306 (id=22905)

Could not rebase; Attempt merge onto c803fc2b59...

Updating c803fc2..7996d10
Fast-forward
 swh/lister/opam/lister.py            | 18 ++++++++----
 swh/lister/opam/tests/test_lister.py | 53 ++++++++++++++++++++++++++++++------
 swh/lister/pattern.py                | 13 +++++++++
 3 files changed, 70 insertions(+), 14 deletions(-)
Changes applied before test
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/360/ for more details.

Harbormaster completed remote builds in B23833: Diff 22905.Sep 20 2021, 5:58 PM
ardumont updated this revision to Diff 22914.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 D6306 (id=22914)

Could not rebase; Attempt merge onto c803fc2b59...

Updating c803fc2..be96c38
Fast-forward
 swh/lister/opam/lister.py            | 20 ++++++++------
 swh/lister/opam/tests/test_lister.py | 53 ++++++++++++++++++++++++++++++------
 swh/lister/pattern.py                | 13 +++++++++
 3 files changed, 70 insertions(+), 16 deletions(-)
Changes applied before test
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/366/ for more details.

Harbormaster completed remote builds in B23842: Diff 22914.Sep 21 2021, 10:40 AM
ardumont updated this revision to Diff 22918.Sep 21 2021, 10:47 AM

rebase

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

Build is green

Patch application report for D6306 (id=22918)

Could not rebase; Attempt merge onto c803fc2b59...

Updating c803fc2..50700fd
Fast-forward
 swh/lister/opam/lister.py            | 20 +++++++------
 swh/lister/opam/tests/test_lister.py | 54 ++++++++++++++++++++++++++++++------
 2 files changed, 58 insertions(+), 16 deletions(-)
Changes applied before test
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/370/ for more details.

Harbormaster completed remote builds in B23846: Diff 22918.Sep 21 2021, 10:50 AM
ardumont updated this revision to Diff 22924.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 D6306 (id=22924)

Could not rebase; Attempt merge onto c803fc2b59...

Updating c803fc2..332ed8e
Fast-forward
 swh/lister/opam/lister.py            | 20 +++++++------
 swh/lister/opam/tests/test_lister.py | 54 ++++++++++++++++++++++++++++++------
 2 files changed, 58 insertions(+), 16 deletions(-)
Changes applied before test
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/375/ for more details.

Harbormaster completed remote builds in B23853: Diff 22924.Sep 21 2021, 11:22 AM
Closed by commit rDLS332ed8e54309: opam: Allow defining where to actually install the opam_root folder (authored by ardumont). · Explain WhySep 21 2021, 11:23 AM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLS332ed8e54309: opam: Allow defining where to actually install the opam_root folder.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
lister/
opam/
4 lines100%
Loading...
tests/
18 lines100%
Loading...

Diff 22918

View Options

swh/lister/opam/lister.py

Loading...
View Options

swh/lister/opam/tests/test_lister.py

Loading...
About · Developer information · Legal