Append path into trimpath if option already exists

[Reflejo]

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Appends paths to trimpath as opposed to override potential used-defined flags.

Which issues(s) does this PR fix?

User defined trimpath through gc_goopts are being overriden since a second -trimpath is 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/)

[achew22]

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?

[Reflejo]

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

[Reflejo]

Ok I added a test case that shows the issue. The test will fail with all_test.go:22: plugin.Open("plugin"): plugin was built with a different version of package go_plugin_with_proto_library/validate without this change.

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 trimpath is needed.

[Reflejo]

@achew22 thoughts?

[robfig]

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!

[RaduBerinde]

The separator in the go code is a semicolon (;), not a colon (:)..

https://github.com/golang/go/blob/e550c3054586a224d949cc8fa030bac0887bee51/src/cmd/internal/objabi/line.go

[RaduBerinde]

Also note that later on, we call absArgs with -trimpath which assumes the argument is a single path. It will not work correctly with the more complex syntax.

https://github.com/bazelbuild/rules_go/blob/ba7bdfd6b5118d135ae3f7984c94103510bf4167/go/tools/builders/compilepkg.go#L476