Page MenuHomeSoftware Heritage
Differential D6340

opam: Define a initialize_opam_root parameter for opam loader
ClosedPublic
Actions

Authored by ardumont on Sep 24 2021, 1:39 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T3590: opam loader: Ensure required opam state is shared amongst ingestion/listing runs
Commits
rDLDBASE8aca257ae8ec: opam: Define a initialize_opam_root parameter for opam loader
Summary

When running in production, the workers should expect the opam root directory to be
present (externally maintained). The production task should do
nothing (initialize_opam_root's default value is False).

When running with standalone loader, they should be instantiated with
initialize_opam_root to True. They will create the opam root folder if not present so it
can work out of the box (e.g. docker worker)

Related to T3590

Test Plan

tox

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Sep 24 2021, 1:39 PM
swh-public-ci added a comment.Sep 24 2021, 1:41 PM

Build is green

Patch application report for D6340 (id=23049)

Could not rebase; Attempt merge onto f4556e0113...

Updating f4556e0..59f469d
Fast-forward
 swh/loader/package/opam/loader.py          | 158 ++++++++++++++++-------------
 swh/loader/package/opam/tests/test_opam.py |  39 ++++++-
 2 files changed, 119 insertions(+), 78 deletions(-)
Changes applied before test
commit 59f469d84d7082f0491055655b1d4bf19fde0d14
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 24 13:37:40 2021 +0200

    opam: Allow deactivation of opam maintenance from loader
    
    This will allow to avoid the extra maintenance step when running in production. The
    default is to let the opam initialization step to be done so standalong loader can work
    out of the box
    
    Related to T3590

commit b2d175965abed6d116a9f70e484ba24ea01f63cc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 20:16:53 2021 +0200

    Allow opam loader to actually use multi-instance opam root
    
    It allow the opam loader to reuse existing opam root with multiple instances. It's the
    complementary code that goes with the loader adaptation [1].
    
    As the `opam show` (cli) [2] version currently packaged does not support the means to
    enclose the metadata extraction per opam instance (when sharing the same opam root), we
    actually work around this by opening internal details to opam.
    
    [1] D6316
    
    [2] `opam show` is currently the interface we are using to extract and list information
    about a package. It does work on standalone opam root folder but it comes short when
    sharing multiple instances within one opam root (for now).

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

Harbormaster completed remote builds in B23976: Diff 23049.Sep 24 2021, 1:41 PM
ardumont requested review of this revision.Sep 24 2021, 1:41 PM
ardumont edited the summary of this revision. (Show Details)Sep 24 2021, 1:48 PM
ardumont updated this revision to Diff 23050.Sep 24 2021, 1:51 PM

Rework commit message, drop spurious changes.

swh-public-ci added a comment.Sep 24 2021, 1:53 PM

Build is green

Patch application report for D6340 (id=23050)

Could not rebase; Attempt merge onto f4556e0113...

Updating f4556e0..2791411
Fast-forward
 swh/loader/package/opam/loader.py          | 158 ++++++++++++++++-------------
 swh/loader/package/opam/tests/test_opam.py |  30 +++++-
 2 files changed, 110 insertions(+), 78 deletions(-)
Changes applied before test
commit 27914116249ca8851bf9cfd9337eaa09404df0a0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 24 13:37:40 2021 +0200

    opam: Allow deactivation of opam maintenance from loader
    
    This will allow to avoid the extra maintenance step when running in production. The
    default is to let the opam initialization step be done by standalone loader so they can
    work out of the box.
    
    Production workers should have that `opam_maintenance` key set to false so they don't
    touch the opam root directory and let the maintenance services do their job [1].
    
    [1] D6305
    Related to T3590

commit b2d175965abed6d116a9f70e484ba24ea01f63cc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 20:16:53 2021 +0200

    Allow opam loader to actually use multi-instance opam root
    
    It allow the opam loader to reuse existing opam root with multiple instances. It's the
    complementary code that goes with the loader adaptation [1].
    
    As the `opam show` (cli) [2] version currently packaged does not support the means to
    enclose the metadata extraction per opam instance (when sharing the same opam root), we
    actually work around this by opening internal details to opam.
    
    [1] D6316
    
    [2] `opam show` is currently the interface we are using to extract and list information
    about a package. It does work on standalone opam root folder but it comes short when
    sharing multiple instances within one opam root (for now).

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

Harbormaster completed remote builds in B23977: Diff 23050.Sep 24 2021, 1:53 PM
ardumont updated this revision to Diff 23051.Sep 24 2021, 1:56 PM

Drop unnecessary changes

swh-public-ci added a comment.Sep 24 2021, 1:58 PM

Build is green

Patch application report for D6340 (id=23051)

Could not rebase; Attempt merge onto f4556e0113...

Updating f4556e0..1ee4276
Fast-forward
 swh/loader/package/opam/loader.py          | 158 ++++++++++++++++-------------
 swh/loader/package/opam/tests/test_opam.py |  41 +++++---
 2 files changed, 110 insertions(+), 89 deletions(-)
Changes applied before test
commit 1ee4276472359289848a3cf0ae02af135e48bf76
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 24 13:55:38 2021 +0200

    test_opam: Drop unnecessary instructions

commit 758269fe640e850091f62a2365a1085a63b9667a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 24 13:37:40 2021 +0200

    opam: Allow deactivation of opam maintenance from loader
    
    This will allow to avoid the extra maintenance step when running in production. The
    default is to let the opam initialization step be done by standalone loader so they can
    work out of the box.
    
    Production workers should have that `opam_maintenance` key set to false so they don't
    touch the opam root directory and let the maintenance services do their job [1].
    
    [1] D6305
    Related to T3590

commit b2d175965abed6d116a9f70e484ba24ea01f63cc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 20:16:53 2021 +0200

    Allow opam loader to actually use multi-instance opam root
    
    It allow the opam loader to reuse existing opam root with multiple instances. It's the
    complementary code that goes with the loader adaptation [1].
    
    As the `opam show` (cli) [2] version currently packaged does not support the means to
    enclose the metadata extraction per opam instance (when sharing the same opam root), we
    actually work around this by opening internal details to opam.
    
    [1] D6316
    
    [2] `opam show` is currently the interface we are using to extract and list information
    about a package. It does work on standalone opam root folder but it comes short when
    sharing multiple instances within one opam root (for now).

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

Harbormaster completed remote builds in B23978: Diff 23051.Sep 24 2021, 1:58 PM
ardumont updated this revision to Diff 23052.Sep 24 2021, 2:00 PM

Use right commit range

swh-public-ci added a comment.Sep 24 2021, 2:03 PM

Build is green

Patch application report for D6340 (id=23052)

Could not rebase; Attempt merge onto f4556e0113...

Updating f4556e0..758269f
Fast-forward
 swh/loader/package/opam/loader.py          | 158 ++++++++++++++++-------------
 swh/loader/package/opam/tests/test_opam.py |  25 +++++
 2 files changed, 110 insertions(+), 73 deletions(-)
Changes applied before test
commit 758269fe640e850091f62a2365a1085a63b9667a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 24 13:37:40 2021 +0200

    opam: Allow deactivation of opam maintenance from loader
    
    This will allow to avoid the extra maintenance step when running in production. The
    default is to let the opam initialization step be done by standalone loader so they can
    work out of the box.
    
    Production workers should have that `opam_maintenance` key set to false so they don't
    touch the opam root directory and let the maintenance services do their job [1].
    
    [1] D6305
    Related to T3590

commit b2d175965abed6d116a9f70e484ba24ea01f63cc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 20:16:53 2021 +0200

    Allow opam loader to actually use multi-instance opam root
    
    It allow the opam loader to reuse existing opam root with multiple instances. It's the
    complementary code that goes with the loader adaptation [1].
    
    As the `opam show` (cli) [2] version currently packaged does not support the means to
    enclose the metadata extraction per opam instance (when sharing the same opam root), we
    actually work around this by opening internal details to opam.
    
    [1] D6316
    
    [2] `opam show` is currently the interface we are using to extract and list information
    about a package. It does work on standalone opam root folder but it comes short when
    sharing multiple instances within one opam root (for now).

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

Harbormaster completed remote builds in B23979: Diff 23052.Sep 24 2021, 2:03 PM
ardumont updated this revision to Diff 23053.Sep 24 2021, 2:05 PM

Adapt docstring to explicit what's said in the commit/diff message/description

swh-public-ci added a comment.Sep 24 2021, 2:07 PM

Build is green

Patch application report for D6340 (id=23053)

Could not rebase; Attempt merge onto f4556e0113...

Updating f4556e0..4a0b2ab
Fast-forward
 swh/loader/package/opam/loader.py          | 180 ++++++++++++++++-------------
 swh/loader/package/opam/tests/test_opam.py |  25 ++++
 2 files changed, 123 insertions(+), 82 deletions(-)
Changes applied before test
commit 4a0b2ab22d8a0d89ea4232145f27a55c437b3252
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 24 13:37:40 2021 +0200

    opam: Allow deactivation of opam maintenance from loader
    
    This will allow to avoid the extra maintenance step when running in production. The
    default is to let the opam initialization step be done by standalone loader so they can
    work out of the box.
    
    Production workers should have that `opam_maintenance` key set to false so they don't
    touch the opam root directory and let the maintenance services do their job [1].
    
    [1] D6305
    Related to T3590

commit b2d175965abed6d116a9f70e484ba24ea01f63cc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 20:16:53 2021 +0200

    Allow opam loader to actually use multi-instance opam root
    
    It allow the opam loader to reuse existing opam root with multiple instances. It's the
    complementary code that goes with the loader adaptation [1].
    
    As the `opam show` (cli) [2] version currently packaged does not support the means to
    enclose the metadata extraction per opam instance (when sharing the same opam root), we
    actually work around this by opening internal details to opam.
    
    [1] D6316
    
    [2] `opam show` is currently the interface we are using to extract and list information
    about a package. It does work on standalone opam root folder but it comes short when
    sharing multiple instances within one opam root (for now).

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

Harbormaster completed remote builds in B23980: Diff 23053.Sep 24 2021, 2:07 PM
ardumont retitled this revision from opam: Allow deactivation of opam maintenance from loader to opam: Define a production mode (default).Sep 27 2021, 11:52 AM
ardumont edited the summary of this revision. (Show Details)
ardumont updated this revision to Diff 23077.Sep 27 2021, 11:53 AM
ardumont edited the summary of this revision. (Show Details)
ardumont added a subscriber: douardda.

Adapt according to discussion with @douardda (production flag is a better name than
maintenance_mode).

swh-public-ci added a comment.Sep 27 2021, 11:55 AM

Build is green

Patch application report for D6340 (id=23077)

Could not rebase; Attempt merge onto f4556e0113...

Updating f4556e0..7b873a0
Fast-forward
 swh/loader/package/opam/loader.py          | 181 ++++++++++++++++-------------
 swh/loader/package/opam/tests/test_opam.py |  50 +++++++-
 2 files changed, 146 insertions(+), 85 deletions(-)
Changes applied before test
commit 7b873a02d6718456951a8330d402bd31a4913890
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 24 13:37:40 2021 +0200

    opam: Define a production mode (default)
    
    When running in production, the workers should expect the opam root directory to be
    present (externally maintained). When running in standalone (non maintenance), the
    loader should create the opam root folder if not present so it can work out of the box.
    
    Docker workers should have that `production` key set to false so they can run in
    standalone mode.
    
    Related to T3590

commit b2d175965abed6d116a9f70e484ba24ea01f63cc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 20:16:53 2021 +0200

    Allow opam loader to actually use multi-instance opam root
    
    It allow the opam loader to reuse existing opam root with multiple instances. It's the
    complementary code that goes with the loader adaptation [1].
    
    As the `opam show` (cli) [2] version currently packaged does not support the means to
    enclose the metadata extraction per opam instance (when sharing the same opam root), we
    actually work around this by opening internal details to opam.
    
    [1] D6316
    
    [2] `opam show` is currently the interface we are using to extract and list information
    about a package. It does work on standalone opam root folder but it comes short when
    sharing multiple instances within one opam root (for now).

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

Harbormaster completed remote builds in B24002: Diff 23077.Sep 27 2021, 11:55 AM
ardumont updated this revision to Diff 23080.Sep 27 2021, 12:13 PM

Adapt according to both suggestions (NewType + explanatory comment)

thanks to both

swh-public-ci added a comment.Sep 27 2021, 12:15 PM

Build is green

Patch application report for D6340 (id=23080)

Rebasing onto 05d6d762d0...

Current branch diff-target is up to date.
Changes applied before test
commit 47d57f12fa674707a2d11def82e5b9a4e17d1cf3
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Sep 26 16:22:16 2021 +0200

    Clarify local/remote heads type as those are hexadecimal bytes str
    
    The current conversions done were a bit ambiguous, specifying the types clarifies the
    need.

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

Harbormaster completed remote builds in B24003: Diff 23080.Sep 27 2021, 12:15 PM
ardumont updated this revision to Diff 23084.Sep 27 2021, 12:16 PM

Update

swh-public-ci added a comment.Sep 27 2021, 12:20 PM

Build is green

Patch application report for D6340 (id=23084)

Could not rebase; Attempt merge onto f4556e0113...

Updating f4556e0..7b873a0
Fast-forward
 swh/loader/package/opam/loader.py          | 181 ++++++++++++++++-------------
 swh/loader/package/opam/tests/test_opam.py |  50 +++++++-
 2 files changed, 146 insertions(+), 85 deletions(-)
Changes applied before test
commit 7b873a02d6718456951a8330d402bd31a4913890
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 24 13:37:40 2021 +0200

    opam: Define a production mode (default)
    
    When running in production, the workers should expect the opam root directory to be
    present (externally maintained). When running in standalone (non maintenance), the
    loader should create the opam root folder if not present so it can work out of the box.
    
    Docker workers should have that `production` key set to false so they can run in
    standalone mode.
    
    Related to T3590

commit b2d175965abed6d116a9f70e484ba24ea01f63cc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 20:16:53 2021 +0200

    Allow opam loader to actually use multi-instance opam root
    
    It allow the opam loader to reuse existing opam root with multiple instances. It's the
    complementary code that goes with the loader adaptation [1].
    
    As the `opam show` (cli) [2] version currently packaged does not support the means to
    enclose the metadata extraction per opam instance (when sharing the same opam root), we
    actually work around this by opening internal details to opam.
    
    [1] D6316
    
    [2] `opam show` is currently the interface we are using to extract and list information
    about a package. It does work on standalone opam root folder but it comes short when
    sharing multiple instances within one opam root (for now).

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

Harbormaster completed remote builds in B24007: Diff 23084.Sep 27 2021, 12:20 PM
douardda added inline comments.Sep 27 2021, 3:29 PM
swh/loader/package/opam/loader.py
65–70

A small detail: this is a bit ambiguous: it makes think there is some "production-mode" detection mechanism that will change the default value for this flag.

(and the actual default value seems to be production=True right?)

vlorentz added a subscriber: vlorentz.Sep 28 2021, 1:02 PM

Not a big fan of calling it production, because it implies it changes a bunch of settings in a non-granual way.

What about an initialize_opam_root or private_opam_root setting instead?

swh/loader/package/opam/loader.py
146–151

isn't this equivalent?

ardumont added a comment.Sep 28 2021, 1:34 PM

Not a big fan of calling it production, because it implies it changes a bunch of settings in a non-granual way.

yes, i don't like it too much. I prefer it over my maintenance_mode which was its initial name though.

What about an initialize_opam_root or private_opam_root setting instead?

I like the former initialize_opam_root! Thanks.

swh/loader/package/opam/loader.py
65–70

is in "production mode" (without the dash) better?

(the default value is True yes).

146–151

looks that way yes ;)

ardumont retitled this revision from opam: Define a production mode (default) to opam: Define a initialize_opam_root parameter for opam loader.Sep 28 2021, 1:49 PM
ardumont edited the summary of this revision. (Show Details)
ardumont updated this revision to Diff 23127.Sep 28 2021, 1:50 PM

Use the right diff id

swh-public-ci added a comment.Sep 28 2021, 1:55 PM

Build is green

Patch application report for D6340 (id=23127)

Could not rebase; Attempt merge onto f4556e0113...

Updating f4556e0..8aca257
Fast-forward
 swh/loader/package/opam/loader.py          | 182 ++++++++++++++++-------------
 swh/loader/package/opam/tests/test_opam.py |  50 +++++++-
 2 files changed, 146 insertions(+), 86 deletions(-)
Changes applied before test
commit 8aca257ae8ec6fe24da0750873528d6252adc92a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 24 13:37:40 2021 +0200

    opam: Define a initialize_opam_root parameter for opam loader
    
    When running in production, the workers should expect the opam root directory to be
    present (externally maintained). The production task should do
    nothing (initialize_opam_root's default value is False).
    
    When running with standalone loader, they should be instantiated with
    initialize_opam_root to True. They will create the opam root folder if not present so it
    can work out of the box (e.g. docker worker)
    
    Related to T3590

commit b2d175965abed6d116a9f70e484ba24ea01f63cc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 21 20:16:53 2021 +0200

    Allow opam loader to actually use multi-instance opam root
    
    It allow the opam loader to reuse existing opam root with multiple instances. It's the
    complementary code that goes with the loader adaptation [1].
    
    As the `opam show` (cli) [2] version currently packaged does not support the means to
    enclose the metadata extraction per opam instance (when sharing the same opam root), we
    actually work around this by opening internal details to opam.
    
    [1] D6316
    
    [2] `opam show` is currently the interface we are using to extract and list information
    about a package. It does work on standalone opam root folder but it comes short when
    sharing multiple instances within one opam root (for now).

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

Harbormaster completed remote builds in B24043: Diff 23127.Sep 28 2021, 1:55 PM
vlorentz accepted this revision.Sep 28 2021, 6:57 PM
This revision is now accepted and ready to land.Sep 28 2021, 6:57 PM
ardumont added inline comments.Sep 28 2021, 6:58 PM
swh/loader/package/opam/loader.py
166

Oh no, now i feel like unifying that as well in the lister...

Closed by commit rDLDBASE8aca257ae8ec: opam: Define a initialize_opam_root parameter for opam loader (authored by ardumont). · Explain WhySep 29 2021, 9:16 AM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDBASE8aca257ae8ec: opam: Define a initialize_opam_root parameter for opam loader.

Revision Contents
Changeset List

PathSize
swh/
loader/
package/
opam/
70 lines
tests/
50 lines

Diff 23158

View Options

swh/loader/package/opam/loader.py

Loading...
View Options

swh/loader/package/opam/tests/test_opam.py

Loading...
About · Developer information · Legal