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

Let go_binary's executable bit depend on linkmode #3143

Merged
merged 1 commit into from
May 9, 2022

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented May 9, 2022

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

If a go_binary's linkmode is neither normal nor pie, the resulting
file can't be executed. In this case, executable should be set to
False on the go_binary rule so that Bazel doesn't create a runfiles
tree for the targets and does not include e.g. a resulting static
library in the runfiles of dependants.

@fmeum fmeum force-pushed the non-executable-linkmodes branch from d7c2284 to 7a8805d Compare May 9, 2022 10:01
go/private/mode.bzl Outdated Show resolved Hide resolved
go/private/mode.bzl Show resolved Hide resolved
If a go_binary's `linkmode` is neither `normal` nor `pie`, the resulting
file can't be executed. In this case, `executable` should be set to
`False` on the go_binary rule so that Bazel doesn't create a runfiles
tree for the targets and does not include e.g. a resulting static
library in the runfiles of dependants.
@fmeum fmeum force-pushed the non-executable-linkmodes branch from 7a8805d to cb27b32 Compare May 9, 2022 18:43
@fmeum
Copy link
Collaborator Author

fmeum commented May 9, 2022

@achew22 Thanks for the review, addressed your comments.

@achew22 achew22 merged commit 7718a1b into bazelbuild:master May 9, 2022
@achew22
Copy link
Member

achew22 commented May 9, 2022

Thanks for your contribution!

fmeum added a commit to CodeIntelligenceTesting/rules_go that referenced this pull request May 12, 2022
If a go_binary is built with a non-executable link mode such as
`c-archive`, its dependents currently pick up a runfile dependency on it
since its DefaultInfo specifies the resulting archive as an executable.
This is unnecessary as the dependent should be able to decide whether to
include the file (e.g. dynamic linking) or not (e.g. static linking).

With this commit, the executable field of the DefaultInfo is only
populated if the go_binary is built with an executable link mode.

Follow-up to bazelbuild#3143
fmeum added a commit to CodeIntelligenceTesting/rules_go that referenced this pull request May 12, 2022
If a go_binary is built with a non-executable link mode such as
`c-archive`, its dependents currently pick up a runfile dependency on it
since its DefaultInfo specifies the resulting archive as an executable.
This is unnecessary as the dependent should be able to decide whether to
include the file (e.g. dynamic linking) or not (e.g. static linking).

With this commit, the executable field of the DefaultInfo is only
populated if the go_binary is built with an executable link mode.

Follow-up to bazelbuild#3143
fmeum added a commit to CodeIntelligenceTesting/rules_go that referenced this pull request May 12, 2022
If a go_binary is built with a non-executable link mode such as
`c-archive`, its dependents currently pick up a runfile dependency on it
since its DefaultInfo specifies the resulting archive as an executable.
This is unnecessary as the dependent should be able to decide whether to
include the file (e.g. dynamic linking) or not (e.g. static linking).

With this commit, the executable field of the DefaultInfo is only
populated if the go_binary is built with an executable link mode.

Follow-up to bazelbuild#3143
fmeum added a commit to CodeIntelligenceTesting/rules_go that referenced this pull request May 12, 2022
If a go_binary is built with a non-executable link mode such as
`c-archive`, its dependents currently pick up a runfile dependency on it
since its DefaultInfo specifies the resulting archive as an executable.
This is unnecessary as the dependent should be able to decide whether to
include the file (e.g. dynamic linking) or not (e.g. static linking).

With this commit, the executable field of the DefaultInfo is only
populated if the go_binary is built with an executable link mode.

Follow-up to bazelbuild#3143
fmeum added a commit to CodeIntelligenceTesting/rules_go that referenced this pull request May 12, 2022
If a go_binary is built with a non-executable link mode such as
`c-archive`, its dependents currently pick up a runfile dependency on it
since its DefaultInfo specifies the resulting archive as an executable.
This is unnecessary as the dependent should be able to decide whether to
include the file (e.g. dynamic linking) or not (e.g. static linking).

With this commit, the executable field of the DefaultInfo is only
populated if the go_binary is built with an executable link mode.

Follow-up to bazelbuild#3143
fmeum added a commit to CodeIntelligenceTesting/rules_go that referenced this pull request May 12, 2022
If a go_binary is built with a non-executable link mode such as
`c-archive`, its dependents currently pick up a runfile dependency on it
since its DefaultInfo specifies the resulting archive as an executable.
This is unnecessary as the dependent should be able to decide whether to
include the file (e.g. dynamic linking) or not (e.g. static linking).

With this commit, the executable field of the DefaultInfo is only
populated if the go_binary is built with an executable link mode.

Follow-up to bazelbuild#3143
fmeum added a commit to CodeIntelligenceTesting/rules_go that referenced this pull request May 12, 2022
If a go_binary is built with a non-executable link mode such as
`c-archive`, its dependents currently pick up a runfile dependency on it
since its DefaultInfo specifies the resulting archive as an executable.
This is unnecessary as the dependent should be able to decide whether to
include the file (e.g. dynamic linking) or not (e.g. static linking).

With this commit, the executable field of the DefaultInfo is only
populated if the go_binary is built with an executable link mode.

Follow-up to bazelbuild#3143
fmeum added a commit to CodeIntelligenceTesting/rules_go that referenced this pull request May 12, 2022
If a go_binary is built with a non-executable link mode such as
`c-archive`, its dependents currently pick up a runfile dependency on it
since its DefaultInfo specifies the resulting archive as an executable.
This is unnecessary as the dependent should be able to decide whether to
include the file (e.g. dynamic linking) or not (e.g. static linking).

With this commit, the executable field of the DefaultInfo is only
populated if the go_binary is built with an executable link mode.

Follow-up to bazelbuild#3143
fmeum added a commit to CodeIntelligenceTesting/rules_go that referenced this pull request May 12, 2022
If a go_binary is built with a non-executable link mode such as
`c-archive`, its dependents currently pick up a runfile dependency on it
since its DefaultInfo specifies the resulting archive as an executable.
This is unnecessary as the dependent should be able to decide whether to
include the file (e.g. dynamic linking) or not (e.g. static linking).

With this commit, the executable field of the DefaultInfo is only
populated if the go_binary is built with an executable link mode.

Follow-up to bazelbuild#3143
linzhp pushed a commit that referenced this pull request Jun 10, 2022
* Remove unnecessary rpath special handling on macOS

Based on
bazelbuild/bazel#10254 (comment)
and bazelbuild/bazel#12304 being fixed, this
special handling of rpaths on macOS appears to be unnecessary.

This cleanup ensures that Bazel sets the correct metadata for the exec
location of Go libraries linked in c-shared mode, which in turn allows
to not include them in the runfiles of all dependents - cc_* targets
depending on them will now generate the correct rpath entries to find
the libraries at runtime at the usual position.

* Don't include non-executable go_binary in dependent's runfiles

If a go_binary is built with a non-executable link mode such as
`c-archive`, its dependents currently pick up a runfile dependency on it
since its DefaultInfo specifies the resulting archive as an executable.
This is unnecessary as the dependent should be able to decide whether to
include the file (e.g. dynamic linking) or not (e.g. static linking).

With this commit, the executable field of the DefaultInfo is only
populated if the go_binary is built with an executable link mode.

Follow-up to #3143
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 this pull request may close these issues.

None yet

2 participants