chore: build noctilucent WASM library in a container #26123

HBobertz · 2023-06-26T16:36:59Z

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

gitpod-io · 2023-06-26T16:37:04Z

vinayak-kukreja · 2023-06-26T18:30:04Z

CONTRIBUTING.md

@@ -64,6 +64,7 @@ The following tools need to be installed on your system prior to installing the
 - [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)


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?

Added a prerequisite test to inform users.

Naumel

I think there's an extra \d*
~~we need to check > 2.5, meaning there is only one digit after the 6-9, before it flips from 2.* to 3.*.~~
the dot is before the 2, not after 👍

Naumel · 2023-06-27T10:07:23Z

scripts/check-build-prerequisites.sh

+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 ]


Here's a railroad diagram of the original (i.e: my) version of the expression (courtesy of https://regexper.com):

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

rix0rrr

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?

RomainMuller · 2023-06-27T10:57:09Z

Rust is already in jsii/superchain, and has been for a LONG while because some Python cryptography module that we transitively depend on needs to be built from it's rust source... I didn't think this would be much of a problem since it's already a host prerequisite in many cases...

I guess we can instead leverage the existing docker dependency instead and do this stuff within a container if you'd prefer that...

rix0rrr · 2023-06-27T14:08:44Z

packages/aws-cdk/generate.sh

+  echo "⏭️ Noctilucent WASM binary is up-to date, skipping build..."
+  echo "ℹ️ Delete lib/vendor/noctilucent/.version to force a rebuild."


❤️ clever

But: wouldn't a git submodule do the same trick, and/or releasing the Noctilucent WASM package to NPMJS by itself?

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

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.

mrgrain · 2023-06-27T19:23:24Z

packages/aws-cdk/generate.sh

-)
+# Build noctilucent package in a Docker/Finch VM
+NOCTILUCENT_GIT="https://github.com/iph/noctilucent.git"
+NOCTILUCENT_COMMIT_ID="6da7c9fade55f8443bba7b8fdfcd4ebfe5208fb1"


I agree with @rix0rrr this feels like we are reinventing versioning and releases.

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...

No longer relevant

mergify · 2023-06-28T08:07:11Z

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-cdk-automation · 2023-06-28T08:35:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 8ea0070
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-06-28T08:35:50Z

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*

add rust dependency to contributing

647bc17

github-actions bot added the p2 label Jun 26, 2023

aws-cdk-automation requested a review from a team June 26, 2023 16:37

Merge branch 'main' into bobertzh/doc-update

eecf5e8

mergify bot added the contribution/core This is a PR that came from AWS. label Jun 26, 2023

HBobertz marked this pull request as ready for review June 26, 2023 16:37

HBobertz changed the title ~~chore: add rust dependency to contributing~~ docs(contributing): add rust dependency Jun 26, 2023

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 26, 2023

vinayak-kukreja reviewed Jun 26, 2023

View reviewed changes

add prerequisite check for rustup

58769b2

Naumel previously requested changes Jun 27, 2023

View reviewed changes

stop trying to regex it propertly

ae51139

RomainMuller force-pushed the bobertzh/doc-update branch from 3a49509 to ae51139 Compare June 27, 2023 10:53

rix0rrr requested changes Jun 27, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 27, 2023

build noctilucent WASM stuff in-container

3609fb2

RomainMuller changed the title ~~docs(contributing): add rust dependency~~ chore: build noctilucent WASM library in a container Jun 27, 2023

rix0rrr reviewed Jun 27, 2023

View reviewed changes

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Jun 27, 2023

HBobertz removed the pr/do-not-merge This PR should not be merged at this time. label Jun 27, 2023

Merge branch 'main' into bobertzh/doc-update

ad45f49

mrgrain reviewed Jun 27, 2023

View reviewed changes

rix0rrr approved these changes Jun 27, 2023

View reviewed changes

HBobertz requested a review from Naumel June 27, 2023 19:35

Merge branch 'main' into bobertzh/doc-update

8ea0070

mergify bot merged commit 561cbc5 into main Jun 28, 2023

mergify bot deleted the bobertzh/doc-update branch June 28, 2023 08:35

ahmetsoykan added a commit to ahmetsoykan/aws-cdk that referenced this pull request Jun 28, 2023

Revert "chore: build noctilucent WASM library in a container (aws#26123…

2d8e9e0

…)" This reverts commit 8bc499c.

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

github-actions bot mentioned this pull request Jul 26, 2023

Monthly PRs metrics report - June 2023 #26516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: build noctilucent WASM library in a container #26123

chore: build noctilucent WASM library in a container #26123

HBobertz commented Jun 26, 2023 •

edited by RomainMuller

Loading

gitpod-io bot commented Jun 26, 2023

vinayak-kukreja Jun 26, 2023

RomainMuller Jun 27, 2023

Naumel left a comment •

edited

Loading

Naumel Jun 27, 2023

RomainMuller Jun 27, 2023

HBobertz Jun 27, 2023 •

edited

Loading

rix0rrr left a comment

RomainMuller commented Jun 27, 2023

rix0rrr Jun 27, 2023

rix0rrr Jun 27, 2023

HBobertz Jun 27, 2023

mrgrain Jun 27, 2023

mrgrain Jun 27, 2023

RomainMuller Jun 28, 2023

mergify bot commented Jun 28, 2023

aws-cdk-automation commented Jun 28, 2023

mergify bot commented Jun 28, 2023

	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 ]

		echo "⏭️ Noctilucent WASM binary is up-to date, skipping build..."
		echo "ℹ️ Delete lib/vendor/noctilucent/.version to force a rebuild."

chore: build noctilucent WASM library in a container #26123

chore: build noctilucent WASM library in a container #26123

Conversation

HBobertz commented Jun 26, 2023 • edited by RomainMuller Loading

gitpod-io bot commented Jun 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naumel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HBobertz Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

RomainMuller commented Jun 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jun 28, 2023

aws-cdk-automation commented Jun 28, 2023

AWS CodeBuild CI Report

mergify bot commented Jun 28, 2023

HBobertz commented Jun 26, 2023 •

edited by RomainMuller

Loading

Naumel left a comment •

edited

Loading

HBobertz Jun 27, 2023 •

edited

Loading