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

Wrap up Nix Build for Node Wrapper #893

Merged
merged 13 commits into from
Aug 8, 2022
Merged

Wrap up Nix Build for Node Wrapper #893

merged 13 commits into from
Aug 8, 2022

Conversation

mikiquantum
Copy link
Contributor

@mikiquantum mikiquantum commented Aug 3, 2022

This PR continues the work from @asymmetric (Thanks!) in this PR

One important thing to note, is that the build process skips building the WASM, this means that the binary and/or docker image needs to use an external spec to work. In later iterations we will add this support.

At the moment images are tagged with a nix-do-not-use suffix until we verify all is fine on production nodes.

A following PR, once we have WASM builds, will cover cleaning up and remove duplication of RUST_TOOLCHAIN in github actions.

@mikiquantum mikiquantum mentioned this pull request Aug 3, 2022
flake.nix Outdated Show resolved Hide resolved
@mikiquantum mikiquantum requested a review from mpontus August 3, 2022 18:11
Copy link
Contributor

@gpmayorga gpmayorga left a comment

Choose a reason for hiding this comment

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

I think we should either remove the Dockerfile(s) and/or rename them and/or move them to a different directory to avoid confusion, as I understand it this replaces the Dockerfile process completely?

@mikiquantum
Copy link
Contributor Author

@gpmayorga the idea is to try this image out in live networks first and then replace all the docker code later. The other reason why we do not remove all that here, is because this binary and docker images doesnt build the wasm, so we cannot use that for our non-live environments. Once we fix that (will create a PR soon) we can do the cleanup.
In any case, I would like to get this merged independently.

@mikiquantum mikiquantum requested a review from gpmayorga August 3, 2022 21:56
flake.nix Show resolved Hide resolved
@mikiquantum mikiquantum requested a review from branan August 4, 2022 23:22
@mikiquantum
Copy link
Contributor Author

@branan & @gpmayorga please have another look and give a thumbsup or ask more questions, so we can get this one in.

@branan
Copy link
Contributor

branan commented Aug 5, 2022

I don't really know Nix well enough to judge this properly, so if everybody else is happy I am. No need to block on me

Copy link
Contributor

@gpmayorga gpmayorga left a comment

Choose a reason for hiding this comment

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

Looks good to me. I like the idea of reading settings from file within the Nix code. I added a question but nothing critical, approving the merge from my end

flake.nix Show resolved Hide resolved
flake.nix Show resolved Hide resolved
@mikiquantum mikiquantum enabled auto-merge (squash) August 8, 2022 19:43
@mikiquantum mikiquantum merged commit 0f6c13b into parachain Aug 8, 2022
@mikiquantum mikiquantum deleted the nix-build branch August 8, 2022 22:32
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.

3 participants