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

fix: release build scripts #1286

Merged
merged 16 commits into from
Apr 19, 2022
Merged

fix: release build scripts #1286

merged 16 commits into from
Apr 19, 2022

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Apr 18, 2022

Closes: #1284

What is the purpose of the change

This change fixes our make build-reproducible Makefile step to generate release binaries. As described in the issue #1284, our current binaries do not have cosm wasm dependencies correctly installed. Wasmvm binaries need to be statically linked.

On initial investigation, I observed that our rbuilder is broken when built locally. The version from docker hub that is used by default when local version is not available has not been updated in around a year and does not work with the main branch of our repository.

As a result, our rbuilder was fixed in this PR by referencing the updated version here: https://github.com/tendermint/images/tree/master/rbuilder

Additionally, it does not make sense to rely on the cosmoshub image (cosmossdk/rbuilder:latest). That is not built by our team.

I propose to push the final result of the rbuilder in this PR to our Dockerhub (i.e osmolabs/rbuilder) CC: @nikever

Brief change log

PR Formatting

Testing and Verifying

Built the binary with make built-reproducible on main branch and successfully ran it without getting the error described in #1284:

./osmosisd-7.0.0-123-g33a436a2-linux-amd64 start
7:09PM INF starting ABCI with Tendermint

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? no
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
  • How is the feature or change documented? added a README in contrib/images/README.md

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2022

Codecov Report

Merging #1286 (3b38167) into main (b6d342a) will decrease coverage by 0.75%.
The diff coverage is 9.32%.

@@            Coverage Diff             @@
##             main    #1286      +/-   ##
==========================================
- Coverage   20.90%   20.14%   -0.76%     
==========================================
  Files         196      203       +7     
  Lines       25425    26824    +1399     
==========================================
+ Hits         5316     5405      +89     
- Misses      19118    20412    +1294     
- Partials      991     1007      +16     
Impacted Files Coverage Δ
x/claim/abci.go 0.00% <ø> (ø)
x/claim/client/cli/query.go 34.45% <ø> (ø)
x/claim/client/cli/tx.go 0.00% <ø> (ø)
x/claim/handler.go 0.00% <ø> (ø)
x/claim/keeper/claim.go 64.33% <ø> (ø)
x/claim/keeper/grpc_query.go 2.50% <ø> (ø)
x/claim/keeper/hooks.go 28.00% <ø> (ø)
x/claim/keeper/keeper.go 87.50% <ø> (ø)
x/claim/keeper/params.go 81.81% <ø> (ø)
x/claim/module.go 58.06% <ø> (ø)
... and 106 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 4e12c8e...3b38167. Read the comment docs.

@p0mvn p0mvn force-pushed the roman/fix-release-builder branch from a3bf439 to d4f95c0 Compare April 18, 2022 22:30
@p0mvn p0mvn changed the title update all builders fix: release build scripts Apr 18, 2022
@p0mvn p0mvn marked this pull request as ready for review April 18, 2022 23:12
Makefile Outdated
@@ -85,7 +88,7 @@ all: install lint test

BUILD_TARGETS := build install

build: BUILD_ARGS=-o $(BUILDDIR)/
build: BUILD_ARGS=-o $(BUILDDIR)/
Copy link
Member

Choose a reason for hiding this comment

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

is this leading space right? I thought make file commands have to have no leading spaces for the definition

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a typo that is fixed now. Thanks for catching that

It worked locally for me. I remember we were having issues in CI when having 4 spaces instead of tabs. That seems to be breaking things

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

This LGTM, but want to leave open in case others who know more than me about the build system get a chance to take a look.

@p0mvn
Copy link
Member Author

p0mvn commented Apr 19, 2022

This LGTM, but want to leave open in case others who know more than me about the build system get a chance to take a look.

Just had a call with @nikever , and he helped me to make the builder logic work with a non-root user in the rbuilder container.

The problem building as a root is that if an end-user runs our binary as a non-root, they might get some permission errors.

I'll make this PR a draft for now to integrate some final changes related to non-root

@p0mvn p0mvn marked this pull request as draft April 19, 2022 14:53
@niccoloraspa niccoloraspa marked this pull request as ready for review April 19, 2022 16:01
@p0mvn
Copy link
Member Author

p0mvn commented Apr 19, 2022

This is ready now

Copy link
Member

@niccoloraspa niccoloraspa left a comment

Choose a reason for hiding this comment

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

LGTM

@ValarDragon ValarDragon merged commit 2029415 into main Apr 19, 2022
@ValarDragon ValarDragon deleted the roman/fix-release-builder branch April 19, 2022 16:50
@faddat faddat mentioned this pull request Apr 19, 2022
4 tasks
@czarcas7ic czarcas7ic added the A:backport/v7.x Do not use. backport patches to v7.x branch label Apr 28, 2022
mergify bot pushed a commit that referenced this pull request Apr 28, 2022
* update all builders

* rbuilder images are prefixed by osmolabs

* update makefile

* use root instead of builder user and other minor changes

* correct lib path in Makefile

* LINK_STATICALLY in Makefile

* docs for builder

* update README

* changelog entry

* remove make clean from .build.sh

* improve readme

* remove spurious whitespace in Makefile

* fix formatting for build-reproducible

* update cosmwasm path in makefile

* copy from /home/builder in build-reproducible

* change wasm file pemissions to non-root

(cherry picked from commit 2029415)

# Conflicts:
#	CHANGELOG.md
czarcas7ic pushed a commit that referenced this pull request Apr 28, 2022
* update all builders

* rbuilder images are prefixed by osmolabs

* update makefile

* use root instead of builder user and other minor changes

* correct lib path in Makefile

* LINK_STATICALLY in Makefile

* docs for builder

* update README

* changelog entry

* remove make clean from .build.sh

* improve readme

* remove spurious whitespace in Makefile

* fix formatting for build-reproducible

* update cosmwasm path in makefile

* copy from /home/builder in build-reproducible

* change wasm file pemissions to non-root
czarcas7ic added a commit that referenced this pull request Apr 28, 2022
* fix: release build scripts (#1286)

* update all builders

* rbuilder images are prefixed by osmolabs

* update makefile

* use root instead of builder user and other minor changes

* correct lib path in Makefile

* LINK_STATICALLY in Makefile

* docs for builder

* update README

* changelog entry

* remove make clean from .build.sh

* improve readme

* remove spurious whitespace in Makefile

* fix formatting for build-reproducible

* update cosmwasm path in makefile

* copy from /home/builder in build-reproducible

* change wasm file pemissions to non-root

(cherry picked from commit 2029415)

# Conflicts:
#	CHANGELOG.md

* fix: release build scripts (#1286)

* update all builders

* rbuilder images are prefixed by osmolabs

* update makefile

* use root instead of builder user and other minor changes

* correct lib path in Makefile

* LINK_STATICALLY in Makefile

* docs for builder

* update README

* changelog entry

* remove make clean from .build.sh

* improve readme

* remove spurious whitespace in Makefile

* fix formatting for build-reproducible

* update cosmwasm path in makefile

* copy from /home/builder in build-reproducible

* change wasm file pemissions to non-root

* fix changelog

Co-authored-by: Roman <34196718+p0mvn@users.noreply.github.com>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
@github-actions github-actions bot mentioned this pull request Jan 15, 2024
@github-actions github-actions bot mentioned this pull request Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v7.x Do not use. backport patches to v7.x branch
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

libwasmvm.so is missing for non-Docker setup
5 participants