Page MenuHomeSoftware Heritage
Differential D6618

Document how each package loader populates fields.
ClosedPublic
Actions

Authored by vlorentz on Nov 8 2021, 1:39 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Maniphest Tasks
T3638: Make package loaders create releases objects instead of revisions
Commits
rDLDBASE054967878093: Document how each package loader populates fields.
Summary

Copied from T3638#73609

Depends on D6616.

Diff Detail

Event Timeline

vlorentz created this revision.Nov 8 2021, 1:39 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptNov 8 2021, 1:39 PM
swh-public-ci added a comment.Nov 8 2021, 1:41 PM

Build has FAILED

Patch application report for D6618 (id=24028)

Could not rebase; Attempt merge onto 5063082e7d...

Updating 5063082..164d296
Fast-forward
 docs/index.rst                                   |   3 +-
 docs/package-loader-specifications.rst           | 106 +++++++
 docs/package-loader-tutorial.rst                 |  25 ++
 swh/loader/package/archive/tests/test_archive.py |  40 +--
 swh/loader/package/cran/tests/test_cran.py       |  37 ++-
 swh/loader/package/debian/tests/test_debian.py   | 106 ++++---
 swh/loader/package/deposit/loader.py             |  25 +-
 swh/loader/package/deposit/tests/test_deposit.py | 134 ++++++---
 swh/loader/package/loader.py                     | 237 +++++++++++----
 swh/loader/package/nixguix/tests/test_nixguix.py |  61 ++--
 swh/loader/package/npm/tests/test_npm.py         | 145 +++++-----
 swh/loader/package/opam/loader.py                |  67 +++--
 swh/loader/package/opam/tests/test_opam.py       | 197 +++++++++++--
 swh/loader/package/pypi/tests/test_pypi.py       | 354 +++++++++--------------
 swh/loader/package/tests/test_loader.py          | 261 +++++++++++++----
 swh/loader/package/tests/test_loader_metadata.py |  13 +-
 swh/loader/tests/__init__.py                     |  18 +-
 swh/loader/tests/test_init.py                    |   4 +-
 18 files changed, 1192 insertions(+), 641 deletions(-)
 create mode 100644 docs/package-loader-specifications.rst
Changes applied before test
commit 164d296f636df503ee227ce18cea9388676a9bde
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon Nov 8 13:39:17 2021 +0100

    Document how each package loader populates fields.

commit 7d7c16822e36eea06f4185521029dbbfd73b5ddb
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Nov 5 13:18:01 2021 +0100

    Make package loaders write releases instead of revisions
    
    The artifacts they load match the semantics of a Release, but we used Revisions
    so far because of technical details (we needed the 'metadata' field of Revision
    that Release lacks) that is no longer relevant (thanks to the metadata storage).
    
    Packages that were loaded by previous versions of the package loader (as revs)
    will be converted to releases. In order to avoid fetching them from the origin,
    the loader will look for an existing extid pointing to a revision (like it used
    to), fetch that revision, extract some fields (directory id, author, date, ...)
    and build a new release using this information.
    
    This commit is unfortunately very large because of all changes in tests, mostly
    just new hashes and renaming 'revision' to 'release' (and various abbreviations
    and capitalizations).
    
    The only meaningful changes are in swh/loader/package/tests/test_loader.py and
    swh/loader/package/loader.py.
    
    To keep this commit as short as possible, I did not yet change individual loaders
    to create releases: they still create revisions, but are converted by the base
    loader. The next commit will refactor them to remove this conversion layer.

commit c0a98a5c4cfe4beac5c0b03bf61459d244b6f132
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 17:58:37 2021 +0100

    tests: Remove duplicate checks
    
    All the '*_missing' tests are already done automatically by check_snapshot
    (it recursively checks all objects are present in the storage).

commit 2311ad9b365e23d1a3c532aa005e32db46bc06f3
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 17:53:20 2021 +0100

    tests: Hide utilities from stack traces
    
    They clutter the test output because pytest prints the whole code
    of the function raising the assertionerror.
    
    With this magic variable, the error is shown as if it was raised
    directly in the caller's body.

commit 551c55ff04774b2c4e50e54f701c514064607f6a
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 15:49:45 2021 +0100

    package loaders: Make test failures more helpful
    
    Some tests did the following:
    
    1. build a snapshot
    2. get the snapshot from the storage
    3. compare it with the expected snapshot
    4. get the origin visit from the storage and check it
    
    If the loader built a wrong snapshot, the test fails at step 2,
    and the only information displayed is that the expected snapshot id
    does not exist, which is very unhelpful.
    
    Instead, I reordered them as: 1, 4, 2, 3. This way, if a wrong
    snapshot is build by the loader, it is detected when comparing
    the visit, and pytest shows the two hashes.
    Then, the test can be modified to use the hash that is actually
    generated to show the actual snapshot.
    
    This is consistent with what was already done in the pypi loader.
    
    Additionally, I made the following changes:
    
    1. always check stats last (because a difference in numbers is
       hardly actionable without testing other objects)
    2. add a few more snapshot id checks in visits
    3. deduplicated a hardcoded snapshot id.

commit 89a0bfee48ca1663faddaa08b8fb3f163d5cfc6f
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Oct 21 14:35:47 2021 +0200

    deposit: Remove 'parent' deposit
    
    The parent is computed by the deposit as the revision of the latest deposit
    in the same origin before the current one.
    Therefore, it is redundant, as it can be recomputed from metadata
    + revision date.
    
    This is a preliminary change needed to make package loaders produce
    releases instead of revisions, as releases don't have parent relationships

commit aeffe01a2b3918262e4bd715ea52c7d7da27807c
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 11:45:36 2021 +0100

    opam: Write package definitions to the extrinsic metadata storage

commit 18bbbae719fc9d165d5b543e54d449f5befc083a
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 11:43:53 2021 +0100

    Add missing documentation for `get_metadata_authority`.

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

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 8 2021, 1:41 PM
Harbormaster failed remote builds in B24902: Diff 24028!
vlorentz edited the summary of this revision. (Show Details)Nov 9 2021, 12:39 PM
vlorentz added a task: T3638: Make package loaders create releases objects instead of revisions.
swh-public-ci added a comment.Nov 9 2021, 12:40 PM

Build has FAILED

Patch application report for D6618 (id=24028)

Could not rebase; Attempt merge onto 5063082e7d...

Updating 5063082..164d296
Fast-forward
 docs/index.rst                                   |   3 +-
 docs/package-loader-specifications.rst           | 106 +++++++
 docs/package-loader-tutorial.rst                 |  25 ++
 swh/loader/package/archive/tests/test_archive.py |  40 +--
 swh/loader/package/cran/tests/test_cran.py       |  37 ++-
 swh/loader/package/debian/tests/test_debian.py   | 106 ++++---
 swh/loader/package/deposit/loader.py             |  25 +-
 swh/loader/package/deposit/tests/test_deposit.py | 134 ++++++---
 swh/loader/package/loader.py                     | 237 +++++++++++----
 swh/loader/package/nixguix/tests/test_nixguix.py |  61 ++--
 swh/loader/package/npm/tests/test_npm.py         | 145 +++++-----
 swh/loader/package/opam/loader.py                |  67 +++--
 swh/loader/package/opam/tests/test_opam.py       | 197 +++++++++++--
 swh/loader/package/pypi/tests/test_pypi.py       | 354 +++++++++--------------
 swh/loader/package/tests/test_loader.py          | 261 +++++++++++++----
 swh/loader/package/tests/test_loader_metadata.py |  13 +-
 swh/loader/tests/__init__.py                     |  18 +-
 swh/loader/tests/test_init.py                    |   4 +-
 18 files changed, 1192 insertions(+), 641 deletions(-)
 create mode 100644 docs/package-loader-specifications.rst
Changes applied before test
commit 164d296f636df503ee227ce18cea9388676a9bde
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon Nov 8 13:39:17 2021 +0100

    Document how each package loader populates fields.

commit 7d7c16822e36eea06f4185521029dbbfd73b5ddb
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Nov 5 13:18:01 2021 +0100

    Make package loaders write releases instead of revisions
    
    The artifacts they load match the semantics of a Release, but we used Revisions
    so far because of technical details (we needed the 'metadata' field of Revision
    that Release lacks) that is no longer relevant (thanks to the metadata storage).
    
    Packages that were loaded by previous versions of the package loader (as revs)
    will be converted to releases. In order to avoid fetching them from the origin,
    the loader will look for an existing extid pointing to a revision (like it used
    to), fetch that revision, extract some fields (directory id, author, date, ...)
    and build a new release using this information.
    
    This commit is unfortunately very large because of all changes in tests, mostly
    just new hashes and renaming 'revision' to 'release' (and various abbreviations
    and capitalizations).
    
    The only meaningful changes are in swh/loader/package/tests/test_loader.py and
    swh/loader/package/loader.py.
    
    To keep this commit as short as possible, I did not yet change individual loaders
    to create releases: they still create revisions, but are converted by the base
    loader. The next commit will refactor them to remove this conversion layer.

commit c0a98a5c4cfe4beac5c0b03bf61459d244b6f132
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 17:58:37 2021 +0100

    tests: Remove duplicate checks
    
    All the '*_missing' tests are already done automatically by check_snapshot
    (it recursively checks all objects are present in the storage).

commit 2311ad9b365e23d1a3c532aa005e32db46bc06f3
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 17:53:20 2021 +0100

    tests: Hide utilities from stack traces
    
    They clutter the test output because pytest prints the whole code
    of the function raising the assertionerror.
    
    With this magic variable, the error is shown as if it was raised
    directly in the caller's body.

commit 551c55ff04774b2c4e50e54f701c514064607f6a
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 15:49:45 2021 +0100

    package loaders: Make test failures more helpful
    
    Some tests did the following:
    
    1. build a snapshot
    2. get the snapshot from the storage
    3. compare it with the expected snapshot
    4. get the origin visit from the storage and check it
    
    If the loader built a wrong snapshot, the test fails at step 2,
    and the only information displayed is that the expected snapshot id
    does not exist, which is very unhelpful.
    
    Instead, I reordered them as: 1, 4, 2, 3. This way, if a wrong
    snapshot is build by the loader, it is detected when comparing
    the visit, and pytest shows the two hashes.
    Then, the test can be modified to use the hash that is actually
    generated to show the actual snapshot.
    
    This is consistent with what was already done in the pypi loader.
    
    Additionally, I made the following changes:
    
    1. always check stats last (because a difference in numbers is
       hardly actionable without testing other objects)
    2. add a few more snapshot id checks in visits
    3. deduplicated a hardcoded snapshot id.

commit 89a0bfee48ca1663faddaa08b8fb3f163d5cfc6f
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Oct 21 14:35:47 2021 +0200

    deposit: Remove 'parent' deposit
    
    The parent is computed by the deposit as the revision of the latest deposit
    in the same origin before the current one.
    Therefore, it is redundant, as it can be recomputed from metadata
    + revision date.
    
    This is a preliminary change needed to make package loaders produce
    releases instead of revisions, as releases don't have parent relationships

commit aeffe01a2b3918262e4bd715ea52c7d7da27807c
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 11:45:36 2021 +0100

    opam: Write package definitions to the extrinsic metadata storage

commit 18bbbae719fc9d165d5b543e54d449f5befc083a
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 11:43:53 2021 +0100

    Add missing documentation for `get_metadata_authority`.

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

ardumont edited the summary of this revision. (Show Details)Nov 9 2021, 1:20 PM
ardumont added a subscriber: ardumont.Nov 9 2021, 1:22 PM

lgtm, couple of remarks inline.

i've added a screenshot as it's easier to read it that way.

docs/package-loader-specifications.rst
35
44
71
vlorentz added inline comments.Nov 9 2021, 1:39 PM
docs/package-loader-specifications.rst
35

that's by design to let the browser reflow; the table is uncomfortable large otherwise.

I should probably replace it with a ZWS, though...

vlorentz updated this revision to Diff 24065.Nov 9 2021, 1:44 PM

use ZWSP instead of actual spaces + fix anchor syntax + use backticks where requested

vlorentz marked 3 inline comments as done.Nov 9 2021, 1:45 PM
swh-public-ci added a comment.Nov 9 2021, 1:46 PM

Build is green

Patch application report for D6618 (id=24065)

Could not rebase; Attempt merge onto 5063082e7d...

Updating 5063082..0549678
Fast-forward
 docs/index.rst                                   |   3 +-
 docs/package-loader-specifications.rst           | 106 +++++++
 docs/package-loader-tutorial.rst                 |  25 ++
 swh/loader/package/archive/tests/test_archive.py |  40 +--
 swh/loader/package/cran/tests/test_cran.py       |  37 ++-
 swh/loader/package/debian/tests/test_debian.py   | 106 ++++---
 swh/loader/package/deposit/loader.py             |  25 +-
 swh/loader/package/deposit/tests/test_deposit.py | 134 ++++++---
 swh/loader/package/loader.py                     | 237 +++++++++++----
 swh/loader/package/nixguix/tests/test_nixguix.py |  61 ++--
 swh/loader/package/npm/tests/test_npm.py         | 145 +++++-----
 swh/loader/package/opam/loader.py                |  67 +++--
 swh/loader/package/opam/tests/test_opam.py       | 197 +++++++++++--
 swh/loader/package/pypi/tests/test_pypi.py       | 353 +++++++++--------------
 swh/loader/package/tests/test_loader.py          | 261 +++++++++++++----
 swh/loader/package/tests/test_loader_metadata.py |  13 +-
 swh/loader/tests/__init__.py                     |  18 +-
 swh/loader/tests/test_init.py                    |   4 +-
 18 files changed, 1191 insertions(+), 641 deletions(-)
 create mode 100644 docs/package-loader-specifications.rst
Changes applied before test
commit 05496787809354552ea9ed0043d687acabf451d0
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon Nov 8 13:39:17 2021 +0100

    Document how each package loader populates fields.

commit 89417bb072aade6b68402dbc10eaee61d98e9ae4
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Nov 5 13:18:01 2021 +0100

    Make package loaders write releases instead of revisions
    
    The artifacts they load match the semantics of a Release, but we used Revisions
    so far because of technical details (we needed the 'metadata' field of Revision
    that Release lacks) that is no longer relevant (thanks to the metadata storage).
    
    Packages that were loaded by previous versions of the package loader (as revs)
    will be converted to releases. In order to avoid fetching them from the origin,
    the loader will look for an existing extid pointing to a revision (like it used
    to), fetch that revision, extract some fields (directory id, author, date, ...)
    and build a new release using this information.
    
    This commit is unfortunately very large because of all changes in tests, mostly
    just new hashes and renaming 'revision' to 'release' (and various abbreviations
    and capitalizations).
    
    The only meaningful changes are in swh/loader/package/tests/test_loader.py and
    swh/loader/package/loader.py.
    
    To keep this commit as short as possible, I did not yet change individual loaders
    to create releases: they still create revisions, but are converted by the base
    loader. The next commit will refactor them to remove this conversion layer.

commit c0a98a5c4cfe4beac5c0b03bf61459d244b6f132
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 17:58:37 2021 +0100

    tests: Remove duplicate checks
    
    All the '*_missing' tests are already done automatically by check_snapshot
    (it recursively checks all objects are present in the storage).

commit 2311ad9b365e23d1a3c532aa005e32db46bc06f3
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 17:53:20 2021 +0100

    tests: Hide utilities from stack traces
    
    They clutter the test output because pytest prints the whole code
    of the function raising the assertionerror.
    
    With this magic variable, the error is shown as if it was raised
    directly in the caller's body.

commit 551c55ff04774b2c4e50e54f701c514064607f6a
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 15:49:45 2021 +0100

    package loaders: Make test failures more helpful
    
    Some tests did the following:
    
    1. build a snapshot
    2. get the snapshot from the storage
    3. compare it with the expected snapshot
    4. get the origin visit from the storage and check it
    
    If the loader built a wrong snapshot, the test fails at step 2,
    and the only information displayed is that the expected snapshot id
    does not exist, which is very unhelpful.
    
    Instead, I reordered them as: 1, 4, 2, 3. This way, if a wrong
    snapshot is build by the loader, it is detected when comparing
    the visit, and pytest shows the two hashes.
    Then, the test can be modified to use the hash that is actually
    generated to show the actual snapshot.
    
    This is consistent with what was already done in the pypi loader.
    
    Additionally, I made the following changes:
    
    1. always check stats last (because a difference in numbers is
       hardly actionable without testing other objects)
    2. add a few more snapshot id checks in visits
    3. deduplicated a hardcoded snapshot id.

commit 89a0bfee48ca1663faddaa08b8fb3f163d5cfc6f
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Oct 21 14:35:47 2021 +0200

    deposit: Remove 'parent' deposit
    
    The parent is computed by the deposit as the revision of the latest deposit
    in the same origin before the current one.
    Therefore, it is redundant, as it can be recomputed from metadata
    + revision date.
    
    This is a preliminary change needed to make package loaders produce
    releases instead of revisions, as releases don't have parent relationships

commit aeffe01a2b3918262e4bd715ea52c7d7da27807c
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 11:45:36 2021 +0100

    opam: Write package definitions to the extrinsic metadata storage

commit 18bbbae719fc9d165d5b543e54d449f5befc083a
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Nov 4 11:43:53 2021 +0100

    Add missing documentation for `get_metadata_authority`.

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

Harbormaster completed remote builds in B24940: Diff 24065.Nov 9 2021, 1:46 PM
vlorentz requested review of this revision.Nov 9 2021, 1:46 PM
vlorentz added inline comments.Nov 9 2021, 1:49 PM
docs/package-loader-specifications.rst
35

Ah, I see the misunderstanding. This is what the table looks like for me, with JS disabled:

ardumont accepted this revision.Nov 9 2021, 4:29 PM
ardumont edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Nov 9 2021, 4:29 PM
vlorentz added a child revision: D6629: Package loader: Uniformize author and message.Nov 9 2021, 4:32 PM
Closed by commit rDLDBASE054967878093: Document how each package loader populates fields. (authored by vlorentz). · Explain WhyNov 10 2021, 2:21 PM
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDLDBASE054967878093: Document how each package loader populates fields..

Revision Contents
Changeset List

PathSize
docs/
3 lines
106 lines

Diff 24065

View Options

docs/index.rst

Loading...
View Options

docs/package-loader-specifications.rst

Loading...
About · Developer information · Legal