Page MenuHomeSoftware Heritage
Differential D5808

add opam lister
ClosedPublic
Actions

Authored by aleo on Jun 1 2021, 4:26 PM.

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 21739
Build 33796: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 33795: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
swh-public-ci added a comment.Jun 3 2021, 12:19 PM

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.
ardumont added a subscriber: ardumont.Jun 4 2021, 10:27 AM

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)

vlorentz requested changes to this revision.Jun 4 2021, 10:45 AM
vlorentz added a subscriber: vlorentz.

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
aleo updated this revision to Diff 20844.Jun 8 2021, 3:22 PM

update the code according to the reviews

swh-public-ci added a comment.Jun 8 2021, 3:22 PM

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
aleo updated this revision to Diff 20873.Jun 9 2021, 12:50 PM

update the code according to the reviews

swh-public-ci added a comment.Jun 9 2021, 12:50 PM

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
swh-public-ci added a comment.Jun 9 2021, 1:03 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.
swh-public-ci added a comment.Jun 9 2021, 2:18 PM

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

ardumont added a comment.Jun 9 2021, 2:29 PM

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

aleo updated this revision to Diff 20881.Jun 9 2021, 3:15 PM

rebase and fix build

swh-public-ci added a comment.Jun 9 2021, 3:18 PM

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
swh-public-ci added a comment.Jun 9 2021, 3:44 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
vlorentz added a comment.Jun 17 2021, 3:13 PM

Do you want help to work on the tests?

aleo added a comment.Jun 23 2021, 1:45 PM

@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
aleo updated this revision to Diff 21243.Jun 23 2021, 5:26 PM

add tests

swh-public-ci added a comment.Jun 23 2021, 5:28 PM

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
aleo added a comment.Jun 23 2021, 5:39 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.

swh-public-ci added a comment.Jun 24 2021, 5:14 PM

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

ardumont added a comment.Jun 24 2021, 5:23 PM

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
ardumont added a comment.Jun 24 2021, 5:24 PM

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.

aleo updated this revision to Diff 21307.Jun 28 2021, 2:46 PM

add missing __init__.py in test to fix build

swh-public-ci added a comment.Jun 28 2021, 2:49 PM

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
ardumont added a comment.Jun 29 2021, 1:44 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.

vlorentz added a comment.EditedJun 29 2021, 1:45 PM

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.

aleo updated this revision to Diff 21497.Jul 5 2021, 5:49 PM

add tasks tests

aleo updated this revision to Diff 21498.Jul 5 2021, 5:50 PM

remove comment

swh-public-ci added a comment.Jul 5 2021, 5:52 PM

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
aleo planned changes to this revision.Jul 5 2021, 5:54 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.

swh-public-ci added a comment.Jul 5 2021, 5:55 PM

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
ardumont added a comment.Jul 5 2021, 5:56 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 ↗(On Diff #21498)

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.

aleo updated this revision to Diff 21500.Jul 5 2021, 6:58 PM

more explanations about opam in the docstring

swh-public-ci added a comment.Jul 5 2021, 7:01 PM

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
aleo updated this revision to Diff 21501.Jul 5 2021, 7:03 PM

add missing copyright header

swh-public-ci added a comment.Jul 5 2021, 7:06 PM

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
aleo updated this revision to Diff 21502.Jul 5 2021, 7:08 PM
aleo marked an inline comment as done.

fix copyright years

aleo marked 7 inline comments as done.Jul 5 2021, 7:10 PM
swh-public-ci added a comment.Jul 5 2021, 7:12 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

ardumont accepted this revision.Jul 6 2021, 9:47 AM

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
aleo updated this revision to Diff 21517.Jul 6 2021, 3:19 PM

add extra_loader_arguments

swh-public-ci added a comment.Jul 6 2021, 3:22 PM

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.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
1 line--
swh/
lister/
opam/
12 lines83%
Loading...
88 lines71%
Loading...
18 lines66%
Loading...
tests/
1 line100%
Loading...

Diff 20778

View Options

setup.py

Loading...
View Options

swh/lister/opam/__init__.py

Loading...
View Options

swh/lister/opam/lister.py

Loading...
View Options

swh/lister/opam/tasks.py

Loading...
View Options

swh/lister/tests/test_cli.py

Loading...
About · Developer information · Legal