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

deps attribute should not use select #1600

Closed
siddharthab opened this issue Jul 22, 2018 · 6 comments
Closed

deps attribute should not use select #1600

siddharthab opened this issue Jul 22, 2018 · 6 comments

Comments

@siddharthab
Copy link
Contributor

bazel's traditional use of command line flags for host and target cpu as global configuration values is not something that go has to limit itself to. As such, the select statement is not a good way of filtering out deps.

go_binary already has a way of specifying goos and goarch, which is then percolated all the way down to stdlib. Instead of specifying deps as a flat list, go_library should use a map of build constraints to deps, and then do the filtering in skylark based on the active build constraints.

A rollout plan could be to allow both types of deps specification, preferring the map over the list, and then slowly phase out the list. gazelle can start output a map by default once rules_go is ready.

@jayconrod
Copy link
Contributor

The reason deps uses select today is that we want to avoid fetching, analyzing, and building dependencies that only build on a platform that is different from the target platform. We rely on select to filter dependencies between the loading and analysis phases. It's good to do this as early as possible to avoid the most work, but unfortunately, that doesn't work with aspects (which is how the goos, goarch and other attributes are implemented).

Your proposal is interesting. That would let us filter dependencies during the analysis phase. So we could avoid building the wrong dependencies (which leads to errors), but we'd still need to fetch and analyze (which just leads to slower builds). This would be compatible with the aspect though (which leads to more correct builds).

I'd rather not implement this because I think we should wait for Bazel to support transitions in Skylark. The latest proposal for that is Skylark Build Configuration.

The reason we support mode attributes on go_binary and go_test is so that we can support multi-platform builds. For example, if you want to build an archive that contains amd64 and arm64 binaries, there's no way to do that today with Bazel --platforms. So as a workaround, you can declare two go_binary rules with the same srcs and deps but different goarch attributes, then package those up with pkg_tar. Personally, I really don't like this solution. Rules in build files should just say what you want to build, not how to build it. I'd much rather to say in pkg_tar srcs: "include this file multiple times, and build for these configurations". Bazel calls this a split transition. It's used in some internal rules (building Android apks with NDK libraries build for multiple architectures), but it's not exposed in Skylark yet.

The proposal above isn't finalized, and I'm sure it's at least a few months away from being implemented. Once it is implemented though, I hope we'll be able to deprecate several of the mode attributes (at least the attributes that would affect dependency selection).

@siddharthab
Copy link
Contributor Author

siddharthab commented Jul 23, 2018

Thank you Jay for your erudite response.

You are right in saying when such a thing will be useful, and also right in saying that this should wait for the proposed features in bazel.

Our multiplatform build use case is to build and push docker images of our binaries such that the tag of the image derived from a status variable, is stamped into another binary through a x_def attribute. If the other binary is being built for anything other than Linux, we have to set goos as linux for the binaries being packaged into docker images. We manage right now by forcefully (#keep in gazelle) repeating the dependencies such that they are the same for all platforms. Having some filtering in skylark instead would have saved us from building the deps unnecessarily, and also kept gazelle happier.

It's not a terrible hack and I am happy that at least it's possible because you have the mode attributes. Can certainly wait for a proper support from bazel.

Thank you again.

@siddharthab
Copy link
Contributor Author

Just a quick question on this topic. We are trying to move from govendor to Go modules, and that means manually editing gazelle generated BUILD files is not so simple anymore.

Is there a way to tell gazelle to not use these select statements and simply include all dependencies in a build? I tried giving -build_tags=linux but that had no effect on the BUILD files.

@jayconrod
Copy link
Contributor

@siddharthab There's no way to do this in Gazelle right now. Doing so would likely lead to broken builds, since you'd be unconditionally depending on targets that may be unbuildable for your target platform.

I do have a bit of good news though: most (possibly all?) of the functionality we need from Starlark Build Configuration is now has merged in Bazel. I need to confirm a design with the Bazel folks and check when all this is released, but we're closer to having working goos and goarch attributes now.

@siddharthab
Copy link
Contributor Author

Thank you!

@jayconrod
Copy link
Contributor

Closing in favor of #2219

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