Details

Reviewers

vlorentz
ardumont

Group Reviewers

Reviewers

Maniphest Tasks

T3358: Opam lister

Commits

rDLSfe01d08cd98f: add opam lister

Required Signatures

L3 Software Heritage Contributor License Agreement, version 1.0

Summary

I added an opam (https://opam.ocaml.org) lister.

I used the opam binary available on most platforms rather than re-implementing opam in python.

As an opam instance can be one of many URL kinds (see http://opam.ocaml.org/doc/Manual.html#URLs) I decided to create virtual URL of the form: opam+INSTANCEURL/package/CURRENTPACKAGE ; in the loader, I'll just have to split this url, get the instance and the package name, query opam for all versions and fetch the tarballs. It should also ease the testing as I'll be able to create a fake opam repository and just use: opam+file://pathtotherepo/packages/.

I'm not familiar with Python so any code style comment is appreciated. Especially the part relative to error handling, I just made a ugly print and exit for now...

Related to T3358

Depends on D5836 (to avoid the most probable build failure unrelated to the diff, most current master builds are failed due to a recent mypy release)

Diff Detail

Repository

rDLS Listers

Branch

add-opam-lister

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 22475
Build 35010: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 35009: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Build is green

Patch application report for D5808 (id=20778)

Rebasing onto 2e0c951be0...

First, rewinding head to replay your work on top of it...
Applying: add opam lister

Changes applied before test

commit 786f5f5d633943ac6cf3836d9005f1330131343f
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

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

Harbormaster completed remote builds in B21739: Diff 20778.Jun 3 2021, 12:19 PM

aleo requested review of this revision.Jun 3 2021, 12:19 PM

ardumont edited the summary of this revision. (Show Details)Jun 3 2021, 3:04 PM

ardumont added a task: T3358: Opam lister.

Thanks!

The python code style looks good to me :)
A couple of suggestions inline.

I'd suggest looking at the cran lister which does use an executable as well.
And have a look at how tests are done for it (i gather mocks).

Cheers,

swh/lister/opam/lister.py
73–78	When it's done, nothing more to list, then let the method exit normally ("do nothing here").
84	It's called an f-string and we tend to use it as it tends to be clearer (ymmv) (That's a nitpick)

Thanks!

Can you add yourself to the CONTRIBUTORS file at the repo root?

And some simple tests, just to make sure it doesn't crash and creates the origins? You can do something like this to mock the external call:

import io
from unittest.mock import MagicMock

def test_something(mocker):
    Popen = Mock()
    Popen.stdout = io.BytesIO(b"line1\nline2\nline3\n")
    mocker.patch("swh.lister.opam.lister.Popen", side_effect=Popen)  # replaces the real Popen with a fake one

    # Then call the lister, see other tests as examples.

swh/lister/opam/lister.py
50–54	Could you use `subprocess` instead of running a shell?

This revision now requires changes to proceed.Jun 4 2021, 10:45 AM

update the code according to the reviews

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/303/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/303/console

Harbormaster failed remote builds in B21806: Diff 20844!Jun 8 2021, 3:22 PM

update the code according to the reviews

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/304/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/304/console

Harbormaster failed remote builds in B21836: Diff 20873!Jun 9 2021, 12:50 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/305/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/305/console

ardumont edited the summary of this revision. (Show Details)Jun 9 2021, 2:17 PM

ardumont added a parent revision: D5836: mypy: Fix errors with release >= v0.900.

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/307/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/307/console

Please, try and rebase this diff upon the latest master which fix another issue (D5836, which you may not hit yet but you will otherwise).

rebase and fix build

Build has FAILED

Patch application report for D5808 (id=20881)

Rebasing onto bf7d44db3c...

Current branch diff-target is up to date.

Changes applied before test

commit a4e58dcf7ffbf55daa251560fc69c7f38afc488e
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/308/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/308/console

Harbormaster failed remote builds in B21849: Diff 20881!Jun 9 2021, 3:19 PM

Build is green

Patch application report for D5808 (id=20881)

Rebasing onto bf7d44db3c...

Current branch diff-target is up to date.

Changes applied before test

commit a4e58dcf7ffbf55daa251560fc69c7f38afc488e
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

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

Harbormaster completed remote builds in B21849: Diff 20881.Jun 9 2021, 3:44 PM

Do you want help to work on the tests?

@vlorentz thanks but it won't be necessary. I was on vacation, I'll update the diff this week with the tests. :)

aleo marked 3 inline comments as done.Jun 23 2021, 3:57 PM

add tests

Build has FAILED

Patch application report for D5808 (id=21243)

Rebasing onto bf7d44db3c...

Current branch diff-target is up to date.

Changes applied before test

commit 652548df321e6a38614a9bba2958fa26f4a54faf
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/310/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/310/console

Harbormaster failed remote builds in B22227: Diff 21243!Jun 23 2021, 5:28 PM

I don't understand the build failure, does someone have any clue ?

There's two simple tests: one which mock the opam call and one which use it for real through a local opam repository. All the files in data are just the local fake opam repository (files are taken from the official instance).

Also, I remember having a discussion about how to make the test_opam_binary function optional (depending on wether the opam command is available or not) but I don't remember how to do it.

Build has FAILED

Patch application report for D5808 (id=21243)

Rebasing onto bf7d44db3c...

Current branch diff-target is up to date.

Changes applied before test

commit 652548df321e6a38614a9bba2958fa26f4a54faf
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/311/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/311/console

I don't understand the build failure, does someone have any clue ?

@aleo I think you are missing a __init__.py emtpy file in swh/lister/opam/tests/__init__.py.
Just add it and git it then update the diff, it should be fine then.

$ cd swh-lister
$ touch swh/lister/opam/tests/__init__.py

I don't understand the build failure, does someone have any clue ?

@aleo I think you are missing a __init__.py emtpy file in swh/lister/opam/tests/__init__.py.
Just add it and git it then update the diff, it should be fine then.
$ cd swh-lister
$ touch swh/lister/opam/tests/__init__.py

That's it, locally it passes now.

add missing __init__.py in test to fix build

Build is green

Patch application report for D5808 (id=21307)

Rebasing onto bf7d44db3c...

Current branch diff-target is up to date.

Changes applied before test

commit 88be612b20333a408cee9fef6931c265db83b677
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

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

Harbormaster completed remote builds in B22288: Diff 21307.Jun 28 2021, 2:49 PM

Almost there, i'd say it's only missing the tests on the task module which i've hinted at inline.

swh/lister/opam/lister.py
79	you can drop the comment now.
swh/lister/opam/tasks.py
13	To test this part, you can look to other listers in their `...tests/<lister-name>/tests/test_tasks.py` module and do something similar. It's mostly to ensure the tasks are correctly registered to avoid surprise down the line when deploying.

I'm a little confused. When/how are all the files in data/fake_opam_repo/ used?

ardumont added inline comments.Jun 29 2021, 1:47 PM

swh/lister/opam/lister.py
63	I'm a little confused. When are all the files in data/fake_opam_repo/ used? @vlorentz I assume they are used here.

ardumont added inline comments.Jun 30 2021, 10:58 AM

swh/lister/opam/tasks.py
13	An example of what i was describing yesterday btw [1] [1] https://forge.softwareheritage.org/source/swh-lister/browse/master/swh/lister/cgit/tests/test_tasks.py$9-35

aleo added inline comments.Jul 5 2021, 5:35 PM

swh/lister/opam/lister.py
63	Actually they're used in the `__init__` method. The url passed for the creation of the opam repository is a local one. Then, when we list all packages here, we get the results from this local repository.

add tasks tests

remove comment

Build is green

Patch application report for D5808 (id=21497)

Rebasing onto bf7d44db3c...

Current branch diff-target is up to date.

Changes applied before test

commit ccda46be7e8a5513c53a4c13a7009e15eb7e550d
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

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

Harbormaster completed remote builds in B22470: Diff 21497.Jul 5 2021, 5:52 PM

Please don't merge now. I noticed while writing the loader that I'll need to add extra arguments to the loader. Will update the diff soon.

Build is green

Patch application report for D5808 (id=21498)

Rebasing onto bf7d44db3c...

Current branch diff-target is up to date.

Changes applied before test

commit 6433c53dc0c012db24c7110343aad2cd10959a6d
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

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

Harbormaster completed remote builds in B22471: Diff 21498.Jul 5 2021, 5:55 PM

Please don't merge now. I noticed while writing the loader that I'll need to add extra arguments to the loader. Will update the diff soon.

We usually let the author do the merge (when the diff is accepted, then git push if the diff is synchronized with the diff, will close the diff).

I added a couple of remarks mostly if not all about copyright headers.

swh/lister/opam/tasks.py
1	2021
swh/lister/opam/tests/test_lister.py
1	Missing copyright headers.

ardumont added inline comments.Jul 5 2021, 6:14 PM

swh/lister/opam/lister.py
26	Can you please give some more details on how the opam command actually does the bootstrap listing? I think that will help understand better what the lister does and also may avoid losing sight of what happens in that lister later. Thanks in advance
26	I meant explain a bit those details within the docstring.

more explanations about opam in the docstring

Build is green

Patch application report for D5808 (id=21500)

Rebasing onto bf7d44db3c...

Current branch diff-target is up to date.

Changes applied before test

commit 7933e9e5f0996f0ce9fe8343e7b316d1ed4e6322
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

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

Harbormaster completed remote builds in B22473: Diff 21500.Jul 5 2021, 7:01 PM

add missing copyright header

Build is green

Patch application report for D5808 (id=21501)

Rebasing onto bf7d44db3c...

Current branch diff-target is up to date.

Changes applied before test

commit 10a3d410770b592e61e37a94cb4b634fa7be12a3
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

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

Harbormaster completed remote builds in B22474: Diff 21501.Jul 5 2021, 7:06 PM

fix copyright years

aleo marked 7 inline comments as done.Jul 5 2021, 7:10 PM

Build is green

Patch application report for D5808 (id=21502)

Rebasing onto bf7d44db3c...

Current branch diff-target is up to date.

Changes applied before test

commit 4f9b5e6126c3e806381f140e19d0e0d214f57b38
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

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

Harbormaster completed remote builds in B22475: Diff 21502.Jul 5 2021, 7:12 PM

vlorentz added inline comments.Jul 6 2021, 12:30 AM

swh/lister/opam/lister.py
63	oh, that's great. thanks

Thanks.

I think we are good now.

I'll let @vlorentz concur or not ;)

vlorentz accepted this revision.Jul 6 2021, 9:53 AM

This revision is now accepted and ready to land.Jul 6 2021, 9:53 AM

add extra_loader_arguments

Build is green

Patch application report for D5808 (id=21517)

Rebasing onto bf7d44db3c...

Current branch diff-target is up to date.

Changes applied before test

commit fe01d08cd98f31136c79096285528c8a5f973faa
Author: zapashcanon <leo@ndrs.fr>
Date:   Mon May 17 15:31:00 2021 +0200

    add opam lister

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

Harbormaster completed remote builds in B22490: Diff 21517.Jul 6 2021, 3:22 PM

Closed by commit rDLSfe01d08cd98f: add opam lister (authored by zapashcanon <leo@ndrs.fr>). · Explain WhyJul 8 2021, 3:47 PM

This revision was automatically updated to reflect the committed changes.

zapashcanon <leo@ndrs.fr> added a commit: rDLSfe01d08cd98f: add opam lister.

add opam listerClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D5808 (id=20778)

Changes applied before test

Patch application report for D5808 (id=20881)

Changes applied before test

Patch application report for D5808 (id=20881)

Changes applied before test

Patch application report for D5808 (id=21243)

Changes applied before test

Patch application report for D5808 (id=21243)

Changes applied before test

Patch application report for D5808 (id=21307)

Changes applied before test

Patch application report for D5808 (id=21497)

Changes applied before test

Patch application report for D5808 (id=21498)

Changes applied before test

Patch application report for D5808 (id=21500)

Changes applied before test

Patch application report for D5808 (id=21501)

Changes applied before test

Patch application report for D5808 (id=21502)

Changes applied before test

Patch application report for D5808 (id=21517)

Changes applied before test

Revision ContentsChangeset List

Diff 21502

CONTRIBUTORS

setup.py

swh/lister/opam/__init__.py

swh/lister/opam/lister.py

swh/lister/opam/tasks.py

swh/lister/opam/tests/__init__.py

swh/lister/opam/tests/data/fake_opam_repo/packages/agrid/agrid.0.1/opam

swh/lister/opam/tests/data/fake_opam_repo/packages/calculon/calculon.0.1/opam

swh/lister/opam/tests/data/fake_opam_repo/packages/calculon/calculon.0.2/opam

swh/lister/opam/tests/data/fake_opam_repo/packages/calculon/calculon.0.3/opam

swh/lister/opam/tests/data/fake_opam_repo/packages/calculon/calculon.0.4/opam

swh/lister/opam/tests/data/fake_opam_repo/packages/calculon/calculon.0.5/opam

swh/lister/opam/tests/data/fake_opam_repo/packages/calculon/calculon.0.6/opam

swh/lister/opam/tests/data/fake_opam_repo/packages/directories/directories.0.1/opam

swh/lister/opam/tests/data/fake_opam_repo/packages/directories/directories.0.2/opam

swh/lister/opam/tests/data/fake_opam_repo/packages/directories/directories.0.3/opam

swh/lister/opam/tests/data/fake_opam_repo/packages/ocb/ocb.0.1/opam

swh/lister/opam/tests/data/fake_opam_repo/repo

swh/lister/opam/tests/data/fake_opam_repo/version

swh/lister/opam/tests/test_lister.py

swh/lister/opam/tests/test_tasks.py

swh/lister/tests/test_cli.py

add opam lister
ClosedPublic
Actions

Revision Contents
Changeset List

swh/lister/opam/init.py

swh/lister/opam/tests/init.py