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

Add build tag to force use of unsafe even with purego? #283

Closed
dsnet opened this issue Dec 23, 2021 · 4 comments · Fixed by #325
Closed

Add build tag to force use of unsafe even with purego? #283

dsnet opened this issue Dec 23, 2021 · 4 comments · Fixed by #325

Comments

@dsnet
Copy link
Collaborator

dsnet commented Dec 23, 2021

In golang/go#23172 (comment), @cespare writes:

It's unfortunate that the best reference for this convention is still this issue, since in the intervening four years no documentation change has been merged.

Additionally, I have found during this time that, in practice, I cannot use the purego tag for its intended purpose in our internal codebase at my company. The reason is that we use go-cmp. Some of go-cmp's core functionality is unsafe. That functionality is now behind a purego build tag. Enabling the purego tag makes most of our tests panic. We have packages with asm as well as pure-Go implementations, and we often want to run automated tests of both code paths, but we cannot run the tests with purego, so we end up using a different build tag to indicate "Go rather than assembly".

I'm not even sure what the right fix is. Maybe the ideal outcome would be that something in the Go standard library (testing? reflect?) would allow go-cmp to do what it needs to do without unsafe. But for now, the existence and popularity of go-cmp kind of "infects" the purego tag and makes it a not-very-useful convention.

I'm not sure what the right fix is, but perhaps an explicit gocmp_unsafe build tag that enables the use of unsafe even if purego is specified? There isn't an obvious fix here since we must use unsafe to introspect unexported fields.

\cc @neild

@cespare
Copy link
Contributor

cespare commented Dec 23, 2021

Maybe it would be best to remove purego from go-cmp. To my mind, the main benefit of this build tag is when you're providing a pure-Go implementation which does the same thing as the unsafe implementation. If the pure-Go implementation is just going to panic anyway, it doesn't seem that useful.

To put it another way, if I were writing code for an environment where unsafe is disallowed (and who does, in 2021?) I would avoid using go-cmp altogether; with or without its purego tag it cannot do what I want it to do without unsafe.

Regarding gocmp_unsafe: having to use a second, gocmp-specific build tag doesn't really seem better than just avoiding purego.

@dsnet
Copy link
Collaborator Author

dsnet commented Dec 23, 2021

Maybe it would be best to remove purego from go-cmp. To my mind, the main benefit of this build tag is when you're providing a pure-Go implementation which does the same thing as the unsafe implementation. If the pure-Go implementation is just going to panic anyway, it doesn't seem that useful.

I'd support that course of action.

It's been a long time, but if I recall correctly, one major motivation behind purego was because a program could not be built for Google AppEngine if it imported unsafe, regardless of whether it was used or not. This was the reason why purego was added to cmp so that AppEngine targets continued to build.

I was not fond of the constraints that AppEngine placed on the rest of the Go ecosystem. By the time I left Google, AppEngine has been on the deprecation chopping block for a long time. I don't know what it's current status is.

@cespare
Copy link
Contributor

cespare commented Nov 18, 2022

Ping @neild. WDYT about removing the use of purego from go-cmp?

cespare added a commit to cespare/go-cmp that referenced this issue Feb 22, 2023
Having go-cmp panic when using the purego build tag makes it hard to use
go-cmp for testing packages that themselves have purego fallbacks.

Since go-cmp can't implement its functionality without unsafe (the
"fallback" panics) and since environments that prohibit unsafe are much
less common these days anyway, simply remove purego code entirely.

Fixes google#283.
@cespare
Copy link
Contributor

cespare commented Feb 22, 2023

Ping again @neild. I went ahead and sent #325.

neild pushed a commit that referenced this issue Feb 22, 2023
Having go-cmp panic when using the purego build tag makes it hard to use
go-cmp for testing packages that themselves have purego fallbacks.

Since go-cmp can't implement its functionality without unsafe (the
"fallback" panics) and since environments that prohibit unsafe are much
less common these days anyway, simply remove purego code entirely.

Fixes #283.
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 a pull request may close this issue.

2 participants