Skip to content

[microsoft/dev.boringcrypto.go1.18] Improve patches #485

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

dagood
Copy link
Member

@dagood dagood commented Mar 16, 2022

  • Improve fix for Fix TestScript/gopath_std_vendor test in microsoft/dev.boringcrypto.go1.18: our additional vendored library interferes #481: don't skip the whole test, address the import issue.
  • Rearrange patches.
    • Make vendoring easier to reproduce by having the patch that adds the imports get applied before the patch that vendors in the library.
    • Better distinguish the changes in each patch. Moved the test fixes together into the last patch and separated out the addition of crypto/internal/backend from the changes throughout the codebase that integrated it by changing import "crypto/internal/boring" to import boring "crypto/internal/backend".

My tools for rearranging were git rebase -i's e mode, and git reset --soft HEAD~1.

Make adding crypto/internal/backend and importing the OpenSSL library happen in the same patch.

Put repo-spanning boring -> OpenSSL changes in its own patch.

Put the vendoring patch after the patch that adds the OpenSSL dependency, for simpler regeneration.

Bring test adjustments/fixes together into the final patch.

In the test adjustment patch, don't skip gopath_std_vendor test, use build tag to remove import
@dagood dagood requested a review from a team as a code owner March 16, 2022 19:27
@dagood dagood requested a review from a user March 16, 2022 19:27
+#
+# See https://github.com/microsoft/go/issues/481 for more details, such as the
+# dependency chain that would cause the failure if the gocrypt tag isn't used.
+go list -tags=gocrypt -test -deps -f '{{.ImportPath}} {{.Dir}}' .
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I had to add -tags=gocrypt to go list, not go build, because this list isn't actually based on the binary. On the plus side: this is a more minimal change. 😄

@dagood dagood requested review from qmuntal, jaredpar and chsienki March 16, 2022 19:31
@dagood
Copy link
Member Author

dagood commented Mar 16, 2022

To verify this PR's changes don't affect the end result, I ran git go-patch apply at the PR's commit and at the 1.18 boring branch, grabbed the two resulting temp commit IDs in the submodule, and diffed them, and it gave what I'd expect--only the test fix improvement:

$ git diff bb913d0f0d 0e82fbf25a
diff --git a/src/cmd/go/testdata/script/gopath_std_vendor.txt b/src/cmd/go/testdata/script/gopath_std_vendor.txt
index 8c39d9a2bd..2d28bd2cf9 100644
--- a/src/cmd/go/testdata/script/gopath_std_vendor.txt
+++ b/src/cmd/go/testdata/script/gopath_std_vendor.txt
@@ -1,7 +1,6 @@
 env GO111MODULE=off
 
 [!gc] skip
-skip 'This test uses a test program that is intended to import no vendored libraries other than golang.org/x/net/http2/hpack, but this is not true in microsoft/go boring 1.18+. https://github.com/microsoft/go/issues/481'
 
 go list -f '{{.Dir}}' vendor/golang.org/x/net/http2/hpack
 stdout $GOPATH[/\\]src[/\\]vendor
@@ -24,7 +23,14 @@ go list -deps -f '{{.ImportPath}} {{.Dir}}' .
 stdout $GOPATH[/\\]src[/\\]vendor[/\\]golang.org[/\\]x[/\\]net[/\\]http2[/\\]hpack
 ! stdout $GOROOT[/\\]src[/\\]vendor
 
-go list -test -deps -f '{{.ImportPath}} {{.Dir}}' .
+# Use build tag 'gocrypt' while evaluating test dependencies to avoid importing
+# vendored crypto module dependencies like go-crypto-openssl. This test script
+# is not set up to handle any vendored libraries being imported other than
+# golang.org/x/net/http2/hpack, so we must make sure it is the only one.
+#
+# See https://github.com/microsoft/go/issues/481 for more details, such as the
+# dependency chain that would cause the failure if the gocrypt tag isn't used.
+go list -tags=gocrypt -test -deps -f '{{.ImportPath}} {{.Dir}}' .
 stdout $GOPATH[/\\]src[/\\]vendor[/\\]golang.org[/\\]x[/\\]net[/\\]http2[/\\]hpack
 ! stdout $GOROOT[/\\]src[/\\]vendor

@dagood
Copy link
Member Author

dagood commented Mar 28, 2022

Also, I think I should actually do the rearranging separately, starting on 1.16 (or 1.17) and merging through to 1.18. That way, all our branches have the same sequence of patches, which will hopefully make future changes easier to merge through the branches (or at least, I won't be making it worse).

Comment on lines +35 to +42
--- a/src/cmd/link/internal/ld/lib.go
+++ b/src/cmd/link/internal/ld/lib.go
@@ -1015,7 +1015,7 @@ var hostobj []Hostobj
// These packages can use internal linking mode.
// Others trigger external mode.
var internalpkg = []string{
- "crypto/internal/boring",
+ "vendor/github.com/microsoft/go-crypto-openssl/openssl",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this doesn't actually seem to be a testing change. Will move it to Integrate OpenSSL module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants