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

Don't use select in srcs #134

Closed
jayconrod opened this issue Feb 20, 2018 · 6 comments
Closed

Don't use select in srcs #134

jayconrod opened this issue Feb 20, 2018 · 6 comments
Labels

Comments

@jayconrod
Copy link
Contributor

Currently, Gazelle identifies platform-specific source files using file name suffixes (e.g., foo_linux_amd64.go) and build tags (e.g., // +build linux,amd64).

It's important that we don't attempt to build packages that are only imported by platform-specific sources that wouldn't get compiled for the target platform. Building the wrong packages causes errors and wastes resources.

That said, there's actually no need to exclude platform-specific sources at analysis time. The compiler wrapper in rules_go also checks build constraints at execution time, so this early exclusion is redundant (even though it saves us a little time).

In some cases (particularly when goos and goarch are set on a go_binary rule), sources that should be included are excluded by select expressions. There's no way to evaluate these expressions correctly in the presence of goos and goarch attributes. We should avoid using them when they're not necessary.

@jayconrod jayconrod added the bug label Feb 20, 2018
jayconrod pushed a commit to jayconrod/bazel-gazelle that referenced this issue Mar 7, 2018
Platform-specfic srcs, separated with select expressions, have caused
a few problems. If the goos and goarch attributes of go_binary are
used, the wrong set of sources may be compiled. rules like go_path
will get filtered lists of sources without any way to get the
unfiltered list.

Separating sources is largely unnecessary, since rules_go also filters
sources at execution time. Therefore, this separation is being removed
by Gazelle.

* Rules generated by gazelle will now have flat srcs lists.
* Existing srcs lists will be flattened by "gazelle update" before
  merging. Sources that appear multiple times will be de-duplicated,
  and comments will be consolidated.

Note that Gazelle still filters sources, it just doesn't generate select
expresions for them anymore. Unbuildable sources (e.g., with
+build ignore) will not appear in any list. Dependencies and flags
will still be platform-specific.

Fixes bazel-contrib#134
jayconrod added a commit that referenced this issue Mar 8, 2018
Platform-specfic srcs, separated with select expressions, have caused
a few problems. If the goos and goarch attributes of go_binary are
used, the wrong set of sources may be compiled. rules like go_path
will get filtered lists of sources without any way to get the
unfiltered list.

Separating sources is largely unnecessary, since rules_go also filters
sources at execution time. Therefore, this separation is being removed
by Gazelle.

* Rules generated by gazelle will now have flat srcs lists.
* Existing srcs lists will be flattened by "gazelle update" before
  merging. Sources that appear multiple times will be de-duplicated,
  and comments will be consolidated.

Note that Gazelle still filters sources, it just doesn't generate select
expresions for them anymore. Unbuildable sources (e.g., with
+build ignore) will not appear in any list. Dependencies and flags
will still be platform-specific.

Fixes #134
@emcfarlane
Copy link

This fixed selection of source files however dependencies deps = [] for go rules are still selected and fail on goos/goarch attributes.

@jayconrod
Copy link
Contributor Author

@afking Unfortunately, there's no practical way to fix that within rules_go or Gazelle. Attributes like goos / goarch on go_binary don't affect the configuration, so select may pick the wrong thing. If we produce flat deps lists instead, we end up building all dependencies, including packages that are unbuildable on the target platform.

@steeve
Copy link
Contributor

steeve commented Mar 13, 2018

There is also the issue of CGo files (.c) being built regardless it seems ?

@jayconrod
Copy link
Contributor Author

@steeve That should not be happening. Could you file an issue on rules_go with steps to reproduce?

Gazelle should include .c files if there is at least one .go file with import "C" and the .c files may be built on at least one platform.

rules_go should filter those files for the target platform based on tags and filename suffixes before attempting to compile them.

@steeve
Copy link
Contributor

steeve commented Mar 13, 2018

I think gazelle successfully skips .c files if no import "C" is found, however, should this file be tagged, the .c file is still built by CGo

@jayconrod
Copy link
Contributor Author

@steeve We pass the full set of go_library sources to the cgo builder for codegen. For each .c file, it checks tags for the target platform. It copies the file if the tags match and creates an empty file if not. We compile the copied / empty files with cc_library.

If there's a case where a .c file should not be matched but is compiled anyway, please file a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants