Page MenuHomeSoftware Heritage

add opam lister
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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.

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

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

update the code according to the reviews

update the code according to the reviews

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

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

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.

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

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

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.

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?

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.

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.

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.

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

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.

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.

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.

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.

aleo marked an inline comment as done.

fix copyright years

aleo marked 7 inline comments as done.Mon, Jul 5, 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.

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

This revision is now accepted and ready to land.Tue, Jul 6, 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.

This revision was automatically updated to reflect the committed changes.