remove dependency on external python for sha256 #1993
Conversation
dragonsinth
requested review from
alexeagle,
gravypod,
pcj and
smukherj1
as code owners
| if err := ioutil.WriteFile(os.Args[2], []byte(hexSum), 0666); err != nil { | ||
| log.Fatalf("error writing %s: %s", os.Args[2], err) | ||
| } | ||
| } |
There was a problem hiding this comment.
| executable = True, | ||
| allow_files = True, | ||
| ), | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
thanks for pointing to that, it's a strange API shape in this file, but I think it's clever how you made it a drop-in replacement.
dragonsinth
changed the title
|
@alexeagle sorry to ping.. I wasn't sure if I'm supposed to do something to make anyone aware I opened this. :) |
| executable = True, | ||
| allow_files = True, | ||
| ), | ||
| } |
There was a problem hiding this comment.
thanks for pointing to that, it's a strange API shape in this file, but I think it's clever how you made it a drop-in replacement.
skylib/hash.bzl
Outdated
| @@ -0,0 +1,23 @@ | |||
| """Functions for producing the hash of an artifact.""" | |||
There was a problem hiding this comment.
could you add a comment here pointing to the upstream hash.bzl in bazel_tools and explain why we forked it? that way we have a chance of comparing this against future changes to that one.
| @@ -87,7 +87,7 @@ A rule that imports a docker image into our intermediate form. | |||
| | <a id="container_import-layers"></a>layers | The list of layer .tar.gz files in the order they appear in the config.json's layer section, or in the order that they appear in the <code>Layers</code> field of the docker save tarballs' <code>manifest.json</code> (these may or may not be gzipped).<br><br> Note that the layers should each have a different basename. | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | required | | | |||
| | <a id="container_import-manifest"></a>manifest | - | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | None | | |||
| | <a id="container_import-repository"></a>repository | - | String | optional | "bazel" | | |||
| | <a id="container_import-sha256"></a>sha256 | - | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | //tools/build_defs/hash:sha256 | | |||
| | <a id="container_import-sha256"></a>sha256 | - | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | //container/go/cmd/sha256:sha256 | | |||
There was a problem hiding this comment.
funny how the old doc here was wrong since it didn't include the repository name
container/go/cmd/sha256/sha256.go
Outdated
| @@ -0,0 +1,39 @@ | |||
| // Portable SHA256 tool. | |||
There was a problem hiding this comment.
Please include a comment here about the sha256.py you mentioned below so we have a chance of finding it later.
|
@alexeagle thanks! Addressed the feedback. |
|
Ping :D |
PR Checklist
Please check if your PR fulfills the following requirements:
^^ unsure if this is necessary; existing tests probably cover?
PR Type
What kind of change does this PR introduce?
What is the current behavior?
sha256 computation depends on https://github.com/bazelbuild/bazel/tree/master/tools/build_defs/hash which requires external python. We can replace this with a simple Go cmd.
Issue Number: #1555
What is the new behavior?
Does this PR introduce a breaking change?