chore: build noctilucent WASM library in a container#26123
chore: build noctilucent WASM library in a container#26123mergify[bot] merged 7 commits intomainfrom
Conversation
|
|
CONTRIBUTING.md
Outdated
| - [Python >= 3.6.5, < 4.0](https://www.python.org/downloads/release/python-365/) | ||
| - [Docker >= 19.03](https://docs.docker.com/get-docker/) | ||
| - the Docker daemon must also be running | ||
| - [Rust >= 1.68, rustup >= 1.25.2](https://www.rust-lang.org/tools/install) |
There was a problem hiding this comment.
Thank you for adding this.
How would users be informed of this change? I think an issue can be created and pinned informing the users about this, WDYT?
There was a problem hiding this comment.
Added a prerequisite test to inform users.
scripts/check-build-prerequisites.sh
Outdated
| check_which $app $app_min | ||
| app_v=$(${app} --version 2>/dev/null) | ||
| echo -e "Checking rustup version... \c" | ||
| if [ $(echo $app_v | grep -c -E "^1\.(2(5\.[2-9]\d*|[6-9]\d*\.\d*)|[3-9]\d+\.\d+)") -eq 1 ] |
There was a problem hiding this comment.
| if [ $(echo $app_v | grep -c -E "^1\.(2(5\.[2-9]\d*|[6-9]\d*\.\d*)|[3-9]\d+\.\d+)") -eq 1 ] | |
| if [ $(echo $app_v | grep -c -E "^1\.(2(5\.[2-9]\d*|[6-9]\.\d*)|[3-9]\d+\.\d+)") -eq 1 ] |
There was a problem hiding this comment.
Here's a railroad diagram of the original (i.e: my) version of the expression (courtesy of https://regexper.com):
There was a problem hiding this comment.
cool site. Based on this it seems romains expression works. Requested elena's re-review just to make sure we're all on the same page
3a49509 to
ae51139
Compare
rix0rrr
left a comment
There was a problem hiding this comment.
I don't think I'm cool with adding this dependency. Contributing is already hard, and this will make it even harder.
Why do we need it, exactly, and why can that not be addressed another way?
|
Rust is already in I guess we can instead leverage the existing |
| echo "⏭️ Noctilucent WASM binary is up-to date, skipping build..." | ||
| echo "ℹ️ Delete lib/vendor/noctilucent/.version to force a rebuild." |
There was a problem hiding this comment.
But: wouldn't a git submodule do the same trick, and/or releasing the Noctilucent WASM package to NPMJS by itself?
There was a problem hiding this comment.
we were originally using git submodules but it turned out codepipeline didn't like that very much so we had to just get it ourselves
There was a problem hiding this comment.
Submodules come with their own set of issues. If it's required for a build, we should not put it into a submodule.
The noctilucent-wasm npm package is still an option.
| ) | ||
| # Build noctilucent package in a Docker/Finch VM | ||
| NOCTILUCENT_GIT="https://github.com/iph/noctilucent.git" | ||
| NOCTILUCENT_COMMIT_ID="6da7c9fade55f8443bba7b8fdfcd4ebfe5208fb1" |
There was a problem hiding this comment.
I agree with @rix0rrr this feels like we are reinventing versioning and releases.
There was a problem hiding this comment.
To clarify - this is just a stepping stone for now... We're trying to avoid wasting time on nom logistics to focus on the product itself. We (the folks working on migrate) are going to own the upkeep of this until it transitions to something our automation is able to deal with...
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Due to the addition of noctilucent to cdk, contributors needed to download rust/rustup to be able to build the cdk. This uses the pre-existing dependency on Docker/Finch to containerize the process in order to not incur any further dependencies for contributors to manage. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…)" This reverts commit 8bc499c.
Due to the addition of noctilucent to cdk, contributors needed to download rust/rustup to be able to build the cdk. This uses the pre-existing dependency on Docker/Finch to containerize the process in order to not incur any further dependencies for contributors to manage. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Due to the addition of noctilucent to cdk, contributors needed to download rust/rustup to be able to build the cdk. This uses the pre-existing dependency on Docker/Finch to containerize the process in order to not incur any further dependencies for contributors to manage. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Due to the addition of noctilucent to cdk, contributors needed to download rust/rustup to be able to build the cdk.
This uses the pre-existing dependency on Docker/Finch to containerize the process in order to not incur any further dependencies for contributors to manage.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license