Skip to content
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

Become go module, reduce deps, add concurrency flag #46

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yunginnanet
Copy link
Contributor

@yunginnanet yunginnanet commented Jun 18, 2024

Summary

This PR implements several fixes and enhancements that allow for a smooth build process and functioning unit test runs in a modern Go environment.

Changes

Enable Go Module1

These days we can't operate out of $GOPATH, as this package used to require to some degree.

Fix this by adding an underscore in the front of the openssl git submodule; then go mod init [...], go mod tidy.

This is because boringssl has Go test runners, so we avoid inheriting those dependencies in the go module.

Ref: https://pkg.go.dev/cmd/go#hdr-Package_lists_and_patterns

Reduce Dependency Footprint2

This remediates failed builds from not having dependencies required strictly for docs, man pages, and such.

Introduce Configurable Build Concurrency3

Hilariously enough on my overclocked i9 if I try to build tor with the previously hardcoded fmt.Sprintf("-j%d", runtime.NumCPU()) - I get sporadic internal gcc errors.

This allows passing -j 16, which in my case, permits me to build tor.

Footnotes

  1. Fix: enable go module by renaming git openssl submodule

  2. Fix: disable building docs to reduce dependency footprint

  3. Feat: Add -j flag to control build concurrency

These days we can't operate out of `$GOPATH`, as this package used to require to some degree.

Fix this by adding an underscore in the front of the `openssl` git submodule.

Ref: https://pkg.go.dev/cmd/go#hdr-Package_lists_and_patterns
Hilariously enough on my overclocked i9 if I try to build tor with the previously  hardcoded `fmt.Sprintf("-j%d", runtime.NumCPU())` - I get sporadic internal gcc errors.

 This allows passing `-j 16`, which in my case permits me to build tor.
@cretz
Copy link
Owner

cretz commented Jun 18, 2024

These days we can't operate out of $GOPATH, as this package used to require to some degree.

Can you help me understand here? So even with modern Go you can't just recursively clone this repo and run go run build.go? Go is simply being used as a build script language, this is not meant to be a Go library

@yunginnanet
Copy link
Contributor Author

yunginnanet commented Jun 18, 2024

These days we can't operate out of $GOPATH, as this package used to require to some degree.

Can you help me understand here? So even with modern Go you can't just recursively clone this repo and run go run build.go? Go is simply being used as a build script language, this is not meant to be a Go library

You can't run go test without this change.

This is because it expects bine to be in your GOPATH unless go modules are enabled, and bine being in our GOPATH without go modules enabled is behavior that the Go team has been slowly deprecating since Go 1.11, and eventually just won't work at all. Since Go 1.16 this breaks your unit test in this repo when ran with a default Go installation/environment.

I respect change being something to shy away from within a project like this where security tends to be a primary consideration. Despite the initial hesitance one may feel because of that; if we dig into it, I don't think there are actually any downsides here.

EDIT: Just realized this does call for some minor modification to bine if we rename the submodule.

@cretz
Copy link
Owner

cretz commented Jun 20, 2024

You can't run go test without this change.

I would support moving just the test to a subfolder and putting the go.mod there so the entire repo doesn't become a Go module. Also, after moving, if it's easier, can just be a binary that does the smoke test instead of utilizing go test.

@yunginnanet
Copy link
Contributor Author

You can't run go test without this change.

I would support moving just the test to a subfolder and putting the go.mod there so the entire repo doesn't become a Go module. Also, after moving, if it's easier, can just be a binary that does the smoke test instead of utilizing go test.

Got it. I'll mark this as a draft until I have time to take a deeper look into this proposed alternative route. I may potentially cherry pick other changes from this PR into new PRs unless you have any quarrels against those to note, @cretz.

@yunginnanet yunginnanet marked this pull request as draft June 22, 2024 11:31
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.

None yet

2 participants