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

incorrect-looking // +build line #261

Closed
rsc opened this issue Mar 31, 2020 · 2 comments
Closed

incorrect-looking // +build line #261

rsc opened this issue Mar 31, 2020 · 2 comments

Comments

@rsc
Copy link

rsc commented Mar 31, 2020

I've been analyzing // +build usage in the Go ecosystem and turned up dom_wasmjs_gopherjs.go, which says:

// +build go1.12,wasm,js js

and dom_native.go which says:

// +build go1.12,!wasm,!js

These two conditions are:

dom_wasmjs_gopherjs.go: (go1.12 && wasm && js) || js which is a complex way of writing js.
dom_native.go: go1.12 && !wasm && !js

I am unsure about what exactly is intended here. From the comments in dom_native.go, it looks like the intent was for dom_wasmjs_gopherjs.go to apply to js/wasm builds and dom_native.go to apply otherwise. That would be:

dom_wasmjs_gopherjs.go: go1.12 && js && wasm

// +build go1.12,js,wasm

dom_native.go: !(go1.12 && js && wasm) which is !go1.12 || !js || !wasm

// +build !go1.12 !js !wasm

Best,
Russ

@slimsag
Copy link
Member

slimsag commented Aug 16, 2020

Thanks Russ and apologies for the delayed response. The intention you described is correct:

it looks like the intent was for dom_wasmjs_gopherjs.go to apply to js/wasm builds and dom_native.go to apply otherwise.

However, it misses one point which was that we are also intending here to support the GopherJS compiler which does not set the wasm build tag. Thus your suggestion go1.12 && js && wasm:

// +build go1.12,js,wasm

Would work in the case of GOOS=wasm GOARCH=js go build but not in the case of gopherjs build which doesn't set the wasm build tag.

You are right that this is true:

dom_wasmjs_gopherjs.go: (go1.12 && wasm && js) || js which is a complex way of writing js.

I wrote it this way in hopes of being a bit more explicit:

  • (go1.12 && wasm && js) -> GOOS=wasm GOARCH=js go build "with Go 1.12+ only"
  • js -> gopherjs build "with any version, because only Go 1.11 is (was) supported in GopherJS"

But given this issue I presume that just js and a comment would've been better perhaps :)

(A bit unrelated, but this is another example of why #264 is probably a good call at this point.)

slimsag added a commit that referenced this issue Aug 16, 2020
* rename files to more clearly denote just JS requirement
* cleanup build tags (debt from legacy GopherJS support) - fixes an issue reported by rsc / Russ Cox in #261
* denote Go 1.14+ requirement with type-checking (possible now due to removing GopherJS support) - fixes #266
@slimsag
Copy link
Member

slimsag commented Aug 16, 2020

Fixed by #267

@slimsag slimsag closed this as completed Aug 16, 2020
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

No branches or pull requests

2 participants