Page MenuHomeSoftware Heritage
Differential D6348

Clarify local/remote heads type as those are hexadecimal bytes str
ClosedPublic
Actions

Authored by ardumont on Sep 27 2021, 10:24 AM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Commits
rDLDGc662603fb825: Clarify local/remote heads type as those are hexadecimal bytes str
Summary

The current conversions done were a bit ambiguous, specifying the types clarifies the
need.

Test Plan

tox

Diff Detail

Event Timeline

ardumont created this revision.Sep 27 2021, 10:24 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 27 2021, 10:24 AM
swh-public-ci added a comment.Sep 27 2021, 10:26 AM

Build is green

Patch application report for D6348 (id=23067)

Rebasing onto 05d6d762d0...

Current branch diff-target is up to date.
Changes applied before test
commit 25cd52608ea214113b24bd3150f92e04766cb4de
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Sep 26 16:40:17 2021 +0200

    Use correct logging instruction, let log.info format entries

commit ffeff53604b4d03b760bc6c3442707fb20f49a99
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Sep 26 16:22:16 2021 +0200

    Drop unnecessary bytes/hex conversion branch.target is already bytes
    
    As per swh.model.model.Snapshot definition

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

Harbormaster completed remote builds in B23994: Diff 23067.Sep 27 2021, 10:26 AM
ardumont requested review of this revision.Sep 27 2021, 10:26 AM
vlorentz requested changes to this revision.Sep 27 2021, 10:36 AM
vlorentz added a subscriber: vlorentz.
vlorentz added inline comments.
swh/loader/git/loader.py
80 ↗(On Diff #23067)

That's not the same behavior.

hashutil.hash_to_hex(branch.target).encode() is equivalent to hashutil.hash_to_bytehex(branch.target); ie. the hexadecimal representation of the hash, but in bytes.

The new code writes the hash digest directly.

This should be caught by tests, though...

This revision now requires changes to proceed.Sep 27 2021, 10:36 AM
ardumont added inline comments.Sep 27 2021, 10:46 AM
swh/loader/git/loader.py
80 ↗(On Diff #23067)

so true... (both)

ardumont added inline comments.Sep 27 2021, 10:55 AM
swh/loader/git/loader.py
80 ↗(On Diff #23067)

I'll explicit the types instead then.

ardumont updated this revision to Diff 23071.Sep 27 2021, 11:29 AM
  • Clarify local/remote heads type as those are hexadecimal bytes str
  • Use correct logging instruction, let log.info format entries
ardumont retitled this revision from Drop unnecessary bytes/hex conversion branch.target is already bytes to Clarify local/remote heads type as those are hexadecimal bytes str.Sep 27 2021, 11:30 AM
ardumont edited the summary of this revision. (Show Details)
swh-public-ci added a comment.Sep 27 2021, 11:31 AM

Build is green

Patch application report for D6348 (id=23071)

Rebasing onto 05d6d762d0...

Current branch diff-target is up to date.
Changes applied before test
commit b09437818e0eb45550df9bbb2d7914d64827539a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sun Sep 26 16:40:17 2021 +0200

    Use correct logging instruction, let log.info format entries

commit bd81a94b2a06fa8afa3a53012383ca17bbc5ede0
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
    
    It was not clear what all those conversion were about. Hopefully, identifying more
    clearly the types help now.

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

Harbormaster completed remote builds in B23997: Diff 23071.Sep 27 2021, 11:31 AM
vlorentz added inline comments.Sep 27 2021, 11:50 AM
swh/loader/git/loader.py
41 ↗(On Diff #23071)

why not a NewType?

ardumont added inline comments.Sep 27 2021, 12:00 PM
swh/loader/git/loader.py
41 ↗(On Diff #23071)

because, you know, that ^ i knew how to do immediately.

Also, before i dedicate more time on this, are you fine with the idea of this type?

douardda added a subscriber: douardda.Sep 27 2021, 12:11 PM
douardda added inline comments.
swh/loader/git/loader.py
41 ↗(On Diff #23071)

Maybe add a comment/docstring to document what this type is/represent?

ardumont added inline comments.Sep 27 2021, 12:13 PM
swh/loader/git/loader.py
41 ↗(On Diff #23071)

Thanks updated with your suggestions to both ;)

ardumont updated this revision to Diff 23081.Sep 27 2021, 12:14 PM
ardumont edited the summary of this revision. (Show Details)

Update

ardumont updated this revision to Diff 23082.Sep 27 2021, 12:14 PM

Fix range commit

ardumont updated this revision to Diff 23083.Sep 27 2021, 12:15 PM

Update

swh-public-ci added a comment.Sep 27 2021, 12:16 PM

Build is green

Patch application report for D6348 (id=23081)

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/118/ for more details.

Harbormaster completed remote builds in B24004: Diff 23081.Sep 27 2021, 12:16 PM
swh-public-ci added a comment.Sep 27 2021, 12:18 PM

Build is green

Patch application report for D6348 (id=23082)

Could not rebase; Attempt merge onto f4556e0113...

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

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] D6316
    
    [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/572/ for more details.

Harbormaster completed remote builds in B24005: Diff 23082.Sep 27 2021, 12:18 PM
swh-public-ci added a comment.Sep 27 2021, 12:18 PM

Build is green

Patch application report for D6348 (id=23083)

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/119/ for more details.

Harbormaster completed remote builds in B24006: Diff 23083.Sep 27 2021, 12:18 PM
ardumont updated this revision to Diff 23141.Sep 28 2021, 3:40 PM

Rebase

swh-public-ci added a comment.Sep 28 2021, 3:42 PM

Build has FAILED

Patch application report for D6348 (id=23141)

Rebasing onto d3976ca650...

Current branch diff-target is up to date.
Changes applied before test
commit 49f74b764c64b3c1617a1fc5b3935aef262e9a08
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.

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

Harbormaster failed remote builds in B24054: Diff 23141!Sep 28 2021, 3:42 PM
ardumont updated this revision to Diff 23144.Sep 28 2021, 4:13 PM

Fix type

swh/loader/git/loader.py
287 ↗(On Diff #23144)

because

swh/loader/git/loader.py:287: error: Argument 1 to "filter_refs" has incompatible type "Dict[bytes, HexBytes]"; expected "Dict[bytes, bytes]"
swh/loader/git/loader.py:287: note: "Dict" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
swh/loader/git/loader.py:287: note: Consider using "Mapping" instead, which is covariant in the value type
Found 1 error in 1 file (checked 16 source files)

and *sigh*

swh-public-ci added a comment.Sep 28 2021, 4:16 PM

Build is green

Patch application report for D6348 (id=23144)

Rebasing onto d3976ca650...

Current branch diff-target is up to date.
Changes applied before test
commit c662603fb8251eda3e3beec8a425b6c21968df1e
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/123/ for more details.

Harbormaster completed remote builds in B24057: Diff 23144.Sep 28 2021, 4:16 PM
vlorentz accepted this revision.Sep 28 2021, 6:54 PM
This revision is now accepted and ready to land.Sep 28 2021, 6:54 PM
Closed by commit rDLDGc662603fb825: Clarify local/remote heads type as those are hexadecimal bytes str (authored by ardumont). · Explain WhySep 28 2021, 7:00 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDGc662603fb825: Clarify local/remote heads type as those are hexadecimal bytes str.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
loader/
package/
opam/
69 lines91%
Loading...
tests/
50 lines100%
Loading...

Diff 23082

View Options

swh/loader/package/opam/loader.py

Loading...
View Options

swh/loader/package/opam/tests/test_opam.py

Loading...
About · Developer information · Legal