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

Support pure = "on" for [email protected] #1629

Closed
wants to merge 1 commit into from

Conversation

mattyclarkson
Copy link
Contributor

@mattyclarkson mattyclarkson commented Mar 15, 2024

This switches out purego build tags for the inverse !cgo build tags.

The correct files are then conditionally added and compiled.

This supports pure = "on" binaries.

A patch adds a pure = "on" unit test to prevent regressions.

@bazel-io
Copy link
Member

Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (circl) have been updated in this PR. Please review the changes.

@mattyclarkson
Copy link
Contributor Author

RFC: @fmeum

@mattyclarkson mattyclarkson force-pushed the circl-is-pure branch 2 times, most recently from 59b9dbe to d3fd88f Compare March 15, 2024 17:01
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fmeum my question here would be: is switching to !cgo the correct thing or should rules_go be setting the purego build tag when pure = "on"?

Copy link
Contributor Author

@mattyclarkson mattyclarkson Mar 18, 2024

Choose a reason for hiding this comment

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

Seems that purego is the community accepted flag: golang/go#23172

Should we drop this and add purego build flag to rules_go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This patch mirrors this comment in that cgo is basically !purego.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the Go thread you linked, I would prefer to add purego support to rules_go instead of moving this module further away from upstream.

Does that sound reasonable to you? Let me know if you want help with the change to rules_go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that sound reasonable to you?

Yeah. That's a better, more general solution assuming that the community aligns on purego. We can always upstream purego tags to projects or patch them in the BCR. I'll get on that patch now


Any views on the noasm flag? Seems that various projects use that to control adding .S files in but I'm unsure what that means for rules_go? If rules_go also specified noasm when pure = "on" then the gazelle generation for circl would likely "Just Work ™️"

Personally, I don't really understand the intersection between cgo, purego and noasm. To myself, they seem mutually exclusive. If purego is on, surely that means !cgo? Also, if you want purego, surely that means noasm as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, the go developers do not include assembly when purego is on: https://go-review.googlesource.com/c/go/+/561935

circl has a separate noasm and purego tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have raised cloudflare/circl#491 for upstream discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cloudflare/circl#492 aligns on purego.

@mattyclarkson mattyclarkson force-pushed the circl-is-pure branch 3 times, most recently from b3d72af to bfbe7ef Compare March 18, 2024 11:40
This switches the `purego` build tag to use the `cgo` tag which
allows the source files to be correctly conditionally included
in the build actions.

There is a second patch that adds a couple of unit tests
that have `pure = "on"` enabled to prevent regressions.
@meteorcloudy meteorcloudy added the presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval label Mar 18, 2024
@meteorcloudy meteorcloudy requested a review from fmeum March 18, 2024 13:25
@mattyclarkson
Copy link
Contributor Author

cloudflare/circl#492 is in 1.3.8
bazel-contrib/bazel-gazelle#1767 understands the purego tag which is in 0.36.0
bazel-contrib/rules_go#3901 sets the purego tag when pure = "on".

Once rules_go gets a release. We won't need a circl BCR module because Gazelle will automatically create the correct BUILD.bazel files for cloudflare/circl.

Closing this.

@mattyclarkson mattyclarkson deleted the circl-is-pure branch May 8, 2024 19:47
@fmeum
Copy link
Contributor

fmeum commented May 9, 2024

@mattyclarkson Wouldn't the module still be needed for the CGO version of circl? Have you observed a difference in performance?

@mattyclarkson
Copy link
Contributor Author

The CGO version will be compiled if pure = "off" (or not specified). i.e. if the purego tag is not provided to the circl build then it will compile in all the assembly.

@fmeum
Copy link
Contributor

fmeum commented May 9, 2024

Sure, but does gazelle generate the correct build files for it to find the assembly without a patch? That's what I introduced the module for in the first place.

@mattyclarkson
Copy link
Contributor Author

You are correct, sorry. It doesn't correctly find the header file. We will still need a patch. I was only testing with pure = "on" 🙄

I'll get one up once the purego friendly rules_go is released.

@mattyclarkson
Copy link
Contributor Author

@fmeum I've opened #1986. GitHub wasn't letting me reopen this one 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants