Append path into trimpath if option already exists#2994
Append path into trimpath if option already exists#2994robfig merged 2 commits intobazel-contrib:masterfrom
Conversation
|
As a rule of thumb, changes to the rules_go compiler infra that aren't accompanied by a regression test are rarely merged. Changes like this are difficult to understand without the context provided by understanding what the current behavior is and what the authors proposed new behavior is. Would you be willing to put a test together to go with this change so we can better understand the problem and, as an added bonus, prevent regressions in the future? |
|
I don't mind writing the tests provided that the maintainers are onboard with the change. That was my plan; the template mentions to submit the PR to discuss test strategies, so that's what I did |
|
Ok I added a test case that shows the issue. The test will fail with This is because the plugin and the host are creating the pb.go file on different build folders which then is included into their binaries. This doesn't work because go plugins are very finicky about dependencies matching exactly and the filepath difference will break it, hence why |
Reflejo
force-pushed
the
fz/fixtrimpath
branch
from
9330f40 to
eb70a28
Compare
Reflejo
force-pushed
the
fz/fixtrimpath
branch
from
eb70a28 to
1c0c559
Compare
|
@achew22 thoughts? |
|
This change looks good to me, and the multi-element trimpath commit was included in Go 1.13 which is old enough that we don't need to worry. Merging, thank you! |
|
The separator in the go code is a semicolon ( |
|
Also note that later on, we call |
**What type of PR is this?** Bug fix **What does this PR do? Why is it needed?** There were multiple problems with trim paths: - The trim path was inconsistent between non-cgo and cgo. non-cgo trimmed the working directory, and cgo trimmed the parent of the working directory. - For non-cgo, paths in external repos are not trimmed when --experimental_sibling_repository_layout is enabled. - For cgo, the name of the working directory is included in the output, but this name is not consistent between local and remote builds, breaking reproducibility. These are fixed here by creating the trim path in the same way for both non-cgo and cgo, and trimming the working directory as well as replacing the parent of the working directory with `..`. This means that the resulting paths are relative to the working directory. Additionally, this fixes the existing feature to extend an existing trimpath. It used the wrong separator (`:` instead of `;`), and it applied `abs()` to the entire trimpath argument instead of the individual paths inside it. (See #2994 (comment) and #2994 (comment)) **Which issues(s) does this PR fix?** Fixes: #4434 Fixes: #4161
4 participants
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
Appends paths to
trimpathas opposed to override potential used-defined flags.Which issues(s) does this PR fix?
User defined
trimpaththroughgc_gooptsare being overriden since a second-trimpathis being set. This change appends the path using:which has been supported by go for a while (see https://go-review.googlesource.com/c/go/+/173344/)