Skip to content
Snippets Groups Projects

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

Test Plan

tox


Migrated from D6340 (view on Phabricator)

Merge request reports

Approval is optional

Closed by Phabricator Migration userPhabricator Migration user 3 years ago (Sep 29, 2021 7:16am UTC)

Merge details

  • The changes were not merged into .

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Build is green

    Patch application report for D6340 (id=23049)

    Could not rebase; Attempt merge onto f4556e01...

    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](https://forge.softwareheritage.org/T3590 'view original for T3590 on Phabricator')
    
    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] swh/devel/swh-lister!406
        
        - [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.

  • Rework commit message, drop spurious changes.

  • Build is green

    Patch application report for D6340 (id=23050)

    Could not rebase; Attempt merge onto f4556e01...

    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] swh/infra/puppet/puppet-swh-site!413
        Related to [T3590](https://forge.softwareheritage.org/T3590 'view original for T3590 on Phabricator')
    
    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] swh/devel/swh-lister!406
        
        - [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.

  • Drop unnecessary changes

  • Build is green

    Patch application report for D6340 (id=23051)

    Could not rebase; Attempt merge onto f4556e01...

    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] swh/infra/puppet/puppet-swh-site!413
        Related to [T3590](https://forge.softwareheritage.org/T3590 'view original for T3590 on Phabricator')
    
    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] swh/devel/swh-lister!406
        
        - [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.

  • Use right commit range

  • Build is green

    Patch application report for D6340 (id=23052)

    Could not rebase; Attempt merge onto f4556e01...

    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] swh/infra/puppet/puppet-swh-site!413
        Related to [T3590](https://forge.softwareheritage.org/T3590 'view original for T3590 on Phabricator')
    
    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] swh/devel/swh-lister!406
        
        - [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.

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

  • Build is green

    Patch application report for D6340 (id=23053)

    Could not rebase; Attempt merge onto f4556e01...

    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] swh/infra/puppet/puppet-swh-site!413
        Related to [T3590](https://forge.softwareheritage.org/T3590 'view original for T3590 on Phabricator')
    
    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] swh/devel/swh-lister!406
        
        - [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.

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

  • Build is green

    Patch application report for D6340 (id=23077)

    Could not rebase; Attempt merge onto f4556e01...

    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](https://forge.softwareheritage.org/T3590 'view original for T3590 on Phabricator')
    
    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] swh/devel/swh-lister!406
        
        - [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.

  • Adapt according to both suggestions (NewType + explanatory comment)

    thanks to both

  • 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.

  • Update

  • Build is green

    Patch application report for D6340 (id=23084)

    Could not rebase; Attempt merge onto f4556e01...

    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](https://forge.softwareheritage.org/T3590 'view original for T3590 on Phabricator')
    
    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] swh/devel/swh-lister!406
        
        - [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.

56 """
57 Load all versions of a given package in a given opam repository.
56 """Load all versions of a given package in a given opam repository.
57
58 The state of the opam repository is stored in a directory called an opam root. This
59 folder is a requisite for the opam binary to actually list information on package.
60
61 When initialize_opam_root is False (the default for production workers), the opam
62 root must already have been configured outside of the loading process. If not an
63 error is raised, thus failing the loading.
58 64
59 The state of the opam repository is stored in a directory called an
60 opam root. Either the opam root has been created by the loader and we
61 simply re-use it, either it doesn't exist yet and we create it on the
62 first package we try to load (next packages will be able to re-use it).
65 For standalone workers, initialize_opam_root must be set to True, so the ingestion
  • 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?)

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

    (the default value is True yes).

  • Please register or sign in to reply
  • vlorentz
    vlorentz @vlorentz started a thread on the diff
  • 150 "--root",
    151 self.opam_root,
    152 self.opam_instance,
    153 self.opam_url,
    154 ]
    155 )
    156 elif not os.path.isfile(os.path.join(self.opam_root, "config")):
    157 raise ValueError("invalid opam root")
    146 if self.initialize_opam_root:
    147 # for standalone loader (e.g docker), loader must initialize the opam root
    148 # folder
    149 call(
    150 [
    151 "opam",
    152 "init",
    153 "--reinit",
  • 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?

  • 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.

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading