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

Added -trimpath option to go build #2064

Merged
merged 4 commits into from
Feb 10, 2020

Conversation

kadern0
Copy link
Contributor

@kadern0 kadern0 commented Feb 10, 2020

Fixes #2063

Signed-off-by: Pablo Caderno kaderno@gmail.com

Which problem is this PR solving?

Short description of the changes

  • Added -trimpath option to go build commands

@kadern0 kadern0 requested a review from a team as a code owner February 10, 2020 04:01
Makefile Outdated
@@ -193,14 +193,14 @@ elasticsearch-mappings:
build-examples:
esc -pkg frontend -o examples/hotrod/services/frontend/gen_assets.go -prefix examples/hotrod/services/frontend/web_assets examples/hotrod/services/frontend/web_assets
ifeq ($(GOARCH), s390x)
CGO_ENABLED=0 installsuffix=cgo go build -o ./examples/hotrod/hotrod-$(GOOS)-$(GOARCH) ./examples/hotrod/main.go
CGO_ENABLED=0 installsuffix=cgo go build -trimpath -o ./examples/hotrod/hotrod-$(GOOS)-$(GOARCH) ./examples/hotrod/main.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be better if we define GOBUILD=CGO_ENABLED=0 installsuffix=cgo go build -trimpath in L30 and use $(GOBUILD), to DRY this up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes total sense. I've updated the code.

@codecov
Copy link

codecov bot commented Feb 10, 2020

Codecov Report

Merging #2064 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2064   +/-   ##
=======================================
  Coverage   97.42%   97.42%           
=======================================
  Files         209      209           
  Lines       10340    10340           
=======================================
  Hits        10074    10074           
  Misses        223      223           
  Partials       43       43

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df16a71...2117ca2. Read the comment docs.

@pavolloffay
Copy link
Member

is -trimpath removing path also from vendored packages? I would prefer to see the full path it helps to distinguish core packages from 3rd party.

Fixes jaegertracing#2063

Signed-off-by: Pablo Caderno <kaderno@gmail.com>
Signed-off-by: kaderno <kaderno@gmail.com>
Defined variable per suggestion of @yurishkuro

Signed-off-by: kaderno <kaderno@gmail.com>
Signed-off-by: Pablo Caderno <kaderno@gmail.com>
Signed-off-by: kaderno <kaderno@gmail.com>
Signed-off-by: kaderno <kaderno@gmail.com>
@yurishkuro
Copy link
Member

@pavolloffay it only trims directory path, not the package name. Vendored packages do not have github.com/jaegertracing/... prefix.

@yurishkuro yurishkuro merged commit a8d76b0 into jaegertracing:master Feb 10, 2020
@yurishkuro
Copy link
Member

Thanks @kadern0

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.

Use go build -trimpath
3 participants