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

Use binaryen-rs as dep instead of requiring manual wasm-opt installation #95

Merged
merged 4 commits into from
Nov 2, 2020

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Oct 30, 2020

Closes #91.

@cmichi cmichi requested a review from ascjones October 30, 2020 16:04
@cmichi
Copy link
Collaborator Author

cmichi commented Oct 30, 2020

The build fails currently in CI while building binaryen-sys, it works locally though. I'm debugging it.

Edit: Found it.

@cmichi cmichi marked this pull request as draft October 30, 2020 21:39
@cmichi cmichi removed the request for review from ascjones October 30, 2020 21:40
@cmichi cmichi force-pushed the cmichi-use-binaryen-dep-instead-of-wasm-opt branch 4 times, most recently from 962183c to f618a90 Compare October 31, 2020 09:35
@cmichi cmichi force-pushed the cmichi-use-binaryen-dep-instead-of-wasm-opt branch from 34d9b64 to 7eee232 Compare October 31, 2020 09:45
@cmichi cmichi marked this pull request as ready for review October 31, 2020 09:53
.gitlab-ci.yml Outdated
@@ -17,6 +17,9 @@ variables:
CARGO_TARGET_DIR: "/ci-cache/${CI_PROJECT_NAME}/targets/${CI_COMMIT_REF_NAME}/${CI_JOB_NAME}"
RUST_LIB_BACKTRACE: "0"

# Necessary for building binaryen-sys, which is part of the binaryen dependency
CXX: "/usr/bin/clang++-8"
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure you want to put it on the top level? Means everything will be linked anew now. Similarly, we could put it in the CI image.

Copy link
Collaborator Author

@cmichi cmichi Nov 2, 2020

Choose a reason for hiding this comment

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

I'm not sure about possible side-effects it would have other projects using this image? Which projects use this base image? Just ink! and cargo-contract?

Regarding top-level: It's needed for all the stages in the file (test, build, fmt), so yeah linking anew is needed for all of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just ink that uses ink-ci-linux image currently. So yeah, if yoou need it everywhere, and if you'd use the CI image as a build local env (as it's possible with vscode), and maybe if someone compiles ink! externally, that'd make sense to introduce this variable to an image.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I created paritytech/scripts#235.

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

❤️ this

.arg("-o")
.arg(optimized.as_os_str())
.output()?;
let codegen_config = binaryen::CodegenConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Job for a follow up, would be good to be able to configure these

@ascjones ascjones merged commit f8f048f into master Nov 2, 2020
@ascjones ascjones deleted the cmichi-use-binaryen-dep-instead-of-wasm-opt branch November 2, 2020 13:04
@TriplEight
Copy link
Contributor

@ascjones don't you need the same change in your CI env?

@cmichi
Copy link
Collaborator Author

cmichi commented Nov 2, 2020

@TriplEight I just updated the scripts PR to include it in both the ink! base image and the cargo-contracts one. I had thought there was only one image for both projects.

@TriplEight
Copy link
Contributor

TriplEight commented Nov 2, 2020

cargo-contract is installed fine to the CI image, it's this what needs to be fixed: paritytech/canvas#21

This was referenced Nov 27, 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.

Fail the build if wasm-opt not installed
3 participants