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

go/build: MatchFile excludes x_js.go when GOOS != js #26424

Closed
neild opened this issue Jul 17, 2018 · 6 comments
Closed

go/build: MatchFile excludes x_js.go when GOOS != js #26424

neild opened this issue Jul 17, 2018 · 6 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Milestone

Comments

@neild
Copy link
Contributor

neild commented Jul 17, 2018

#102835 adds "js" to build.goosList, causing build.Context.MatchFile to exclude files named *_js.go when GOOS is not "js".

We have a number of existing files that match that pattern (mostly generated code containing embedded JavaScript strings), and there are probably more in the world at large. This seems like it might be too common a suffix to change at this time.

@rsc rsc added this to the Go1.11 milestone Jul 17, 2018
@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 17, 2018
@ianlancetaylor
Copy link
Contributor

Approximately a dup of #26329, though there we just documented this change.

If we change this, presumably by removing js from goosList in go/build/syslist.go, there are 13 xxx_js.go files in the standard library that may need explicit build tags.

We could consider changing the GOOS name from js to something less common. But the use of build tags in file names is an ongoing problem that will bite us again in the future. We could simply freeze the current set, and require explicit build tags going forward.

CC @neelance @bradfitz

@rsc
Copy link
Contributor

rsc commented Jul 17, 2018

The specific case where this arises in Google is that we have a go-bindata-like tool that reads x.js and writes x_js.go. Looking at the public go-bindata, though, it just writes one file bindata.go, which doesn't hit this problem. I'm also looking through a very large GOPATH with tons of packages pulled in, and it looks like there are at least a few external projects that already had *_js.go files for use with GopherJS.

It may be that we can leave this as-is and document. I'll report back once I have more data (still downloading).

@cespare
Copy link
Contributor

cespare commented Jul 17, 2018

I think we should explicitly and prominently document that naming files with underscores that aren't instructions to go build is discouraged and likely to lead to broken code in the future. I think this has been apparent to a lot of us for a long time now.

Personally, I really like the feature of naming files with os/arch and I'd dislike it if we moved in the direction of requiring explicit build tags in more situations.

@bradfitz
Copy link
Contributor

FWIW we did document this in the release notes: https://tip.golang.org/doc/go1.11#wasm

@neild
Copy link
Contributor Author

neild commented Jul 17, 2018

I think we should explicitly and prominently document that naming files with underscores that aren't instructions to go build is discouraged and likely to lead to broken code in the future.

If we go that route then I would argue that Go 2 should always interpret files with underscores as containing build instructions, regardless of the current set of GOOS values.

@rsc
Copy link
Contributor

rsc commented Jul 17, 2018

I am not convinced we should disallow all use of underscores except for GOOS/GOARCH. I think it would be fine for a project to have storage_aws.go, storage_azure.go, etc. If we do put the go version into the module file (#23969) then the go command could set the GOOS/GOARCH list appropriately when looking at files in that module. That would future-proof things (at least as far as not breaking builds) without imposing additional requirements on users.

Back to the question of how common the _js suffix is.

I have a list of the most commonly imported ~1000 open source repos that I use for testing vgo, plus ~100 of the projects that import the most other things. Scanning those repos and their dependencies, I found:

  • 80,931 packages (29,281 after removing vendored code)
  • 412,486 files (143,346 after removing vendored code)
  • 51 *_js.go files (8 after removing vendored code)

It looks like these are all for GopherJS or the new wasm/js support:

$ find . -name '*_js.go' | grep -v /vendor/
./github.com/jtolds/gls/stack_tags_js.go
./github.com/pborman/uuid/node_js.go
./github.com/google/uuid/node_js.go
./github.com/gogo/protobuf/test/int64support/object_js.go
./github.com/goadesign/goa/uuid/uuid_js.go
./github.com/goadesign/goa/metrics_js.go
./github.com/mattn/go-runewidth/runewidth_js.go
./github.com/gopherjs/gopherjs/compiler/natives/src/internal/poll/fd_poll_js.go
$ 

That suggests it's fine to document and close (already done).

@rsc rsc closed this as completed Jul 17, 2018
@golang golang locked and limited conversation to collaborators Jul 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants