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

Build binaries for Linux on IBM Z / s390x architecture #1982

Merged
merged 3 commits into from
Jan 13, 2020

Conversation

prankkelkar
Copy link
Contributor

Signed-off-by: Prasanna Kelkar Prasanna.Kelkar@ibm.com

Which problem is this PR solving?

  • PR not intended to resolve specific issue but As Travis CI officially supports s390x builds, adding support for same.

Short description of the changes

  • Added s390x specific changes to skip usage of RACE flag as it is not supported on s390x architecture.
  • Added type casting in plugin/storage/badger/stats_linux.go file to resolve mismatched types int64 and uint32 issue on s390x platform.

Signed-off-by: Prasanna Kelkar <Prasanna.Kelkar@ibm.com>
@jpkrohling
Copy link
Contributor

Please, see #1973. There are two main problems:

  1. the extra time required to perform a build. Instead of running the whole build in a new arch, only the fast unit-tests should be executed
  2. what happens if there's a new problem in the future? Are you able to commit to support this when it needs maintenance? The current maintainers do not have the necessary bandwidth to support arches that aren't used by ourselves...

@codecov
Copy link

codecov bot commented Dec 18, 2019

Codecov Report

Merging #1982 into master will increase coverage by 0.38%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1982      +/-   ##
==========================================
+ Coverage   96.99%   97.38%   +0.38%     
==========================================
  Files         203      206       +3     
  Lines       10064    10136      +72     
==========================================
+ Hits         9762     9871     +109     
+ Misses        264      222      -42     
- Partials       38       43       +5
Impacted Files Coverage Δ
plugin/storage/badger/stats_linux.go 100% <100%> (ø) ⬆️
pkg/queue/bounded_queue.go 94.8% <0%> (-5.2%) ⬇️
cmd/query/app/static_handler.go 86.84% <0%> (-1.76%) ⬇️
plugin/storage/cassandra/spanstore/reader.go 100% <0%> (ø) ⬆️
cmd/collector/app/builder/span_handler_builder.go 100% <0%> (ø) ⬆️
...in/sampling/strategystore/static/strategy_store.go 100% <0%> (ø) ⬆️
cmd/agent/app/agent.go 100% <0%> (ø) ⬆️
cmd/builder/builder_options.go
cmd/agent/app/httpserver/server.go
cmd/agent/app/httpserver/srv.go 100% <0%> (ø)
... and 6 more

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 46d7f17...ac1d0ae. Read the comment docs.

Signed-off-by: Prasanna Kelkar <Prasanna.Kelkar@ibm.com>
@prankkelkar
Copy link
Contributor Author

Please, see #1973. There are two main problems:

  1. the extra time required to perform a build. Instead of running the whole build in a new arch, only the fast unit-tests should be executed
  2. what happens if there's a new problem in the future? Are you able to commit to support this when it needs maintenance? The current maintainers do not have the necessary bandwidth to support arches that aren't used by ourselves...
  1. I have modified the changes as per you instructions to run only unit tests for s390x architecture.
  2. We already build and maintain jaeger on s390x. Our build instructions are published here

@yurishkuro
Copy link
Member

We're currently not running any tests on Windows or MacOS platforms, we only build the binaries for them. Could we do the same for this arch? This would have minimal impact on the CI time.

@prankkelkar
Copy link
Contributor Author

Yes. It will be great if we can have binaries released for s390x architecture which could be downloaded directly. I believe changes in Makefile to add target for s390x in build-all-platforms should be enough. Please let me know if any more changes are needed.

We're currently not running any tests on Windows or MacOS platforms, we only build the binaries for them. Could we do the same for this arch? This would have minimal impact on the CI time.

@prankkelkar
Copy link
Contributor Author

prankkelkar commented Jan 8, 2020

@yurishkuro Could you please guide me with the changes needed to release binaries for s390x ?

@yurishkuro
Copy link
Member

Follow the path of make build-all-platforms in the Makefile.

@prankkelkar
Copy link
Contributor Author

@yurishkuro While i was adding linux s390x support i came across the names of the binaries used. for eg. collector/collector-$(GOOS)
In the case of s390x as the os name is same i.e Linux, two binaries will be created with the same name as per current code. To resolve this issue I was planning to add $(GOARCH) only to s390x binary name to differentiate it from amd64 linux.
Is there any other naming conventions we have to follow?

@yurishkuro
Copy link
Member

Could we manage that at the directory name level, and keep the file names as is?

We may also consider not using OS in the binary name, only in the directory.

cc @jpkrohling @pavolloffay @objectiser

@jpkrohling
Copy link
Contributor

Minikube adds both to the binaries:

minikube-linux-amd64
minikube-windows-amd64.exe
minikube-darwin-amd64

In our case, it would probably be too noisy, so, the idea of grouping them by OS makes sense.

@prankkelkar
Copy link
Contributor Author

I have currently added following code to handle this scenario

.PHONY: build-binaries-s390x
build-binaries-s390x:
	GOOS=linux GOARCH=s390x $(MAKE) build-platform-binaries

.PHONY: build-agent
build-agent:
ifeq ($(GOARCH), s390x)
	CGO_ENABLED=0 installsuffix=cgo go build -o ./cmd/agent/agent-$(GOOS)-$(GOARCH) $(BUILD_INFO) ./cmd/agent/main.go
else
	CGO_ENABLED=0 installsuffix=cgo go build -o ./cmd/agent/agent-$(GOOS) $(BUILD_INFO) ./cmd/agent/main.go
endif

This will only change the name of the binary of s390x and rest will remain as it is.
I have also tweaked ./scripts/travis/package-deploy.sh to handle this change. Should i go ahead to update PR with this approach?

@yurishkuro
Copy link
Member

sgtm

Signed-off-by: Prasanna Kelkar <Prasanna.Kelkar@ibm.com>
@prankkelkar
Copy link
Contributor Author

@yurishkuro I have updated the PR to cross compile and release the binaries. Please review the code and let me know if anything needs to be changed.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Looks reasonable, thanks.

@yurishkuro yurishkuro changed the title Add s390x Travis support Build binaries for Linux on IBM Z / s390x architecture Jan 13, 2020
@yurishkuro yurishkuro merged commit 7d8a2d4 into jaegertracing:master Jan 13, 2020
@pavolloffay pavolloffay added this to the Release 1.17 milestone Jan 14, 2020
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.

4 participants