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

chore: add prerequisite check before build #8929

Merged
merged 15 commits into from
Jul 14, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ you need to have the following SDKs and tools locally:
- We recommend using a version in [Active LTS](https://nodejs.org/en/about/releases/)
- ⚠️ versions `13.0.0` to `13.6.0` are not supported due to compatibility issues with our dependencies.
- [Yarn >= 1.19.1](https://yarnpkg.com/lang/en/docs/install)
- [Java OpenJDK 8](https://docs.aws.amazon.com/corretto/latest/corretto-8-ug/downloads-list.html)
- [Java >= OpenJDK 8](https://docs.aws.amazon.com/corretto/latest/corretto-8-ug/downloads-list.html)
- [Apache Maven](http://maven.apache.org/install.html)
- [.NET Core SDK 3.1](https://www.microsoft.com/net/download)
- [Python 3.6.5](https://www.python.org/downloads/release/python-365/)
Expand All @@ -89,7 +89,6 @@ The basic commands to get the repository cloned and built locally follow:
```console
$ git clone https://github.com/aws/aws-cdk.git
$ cd aws-cdk
$ yarn install
$ yarn build
```

Expand Down
4 changes: 4 additions & 0 deletions build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,12 @@ fail() {
exit 1
}

# Check for secrets that should not be committed
/bin/bash ./git-secrets-scan.sh

# Verify dependencies before starting the build
/bin/bash ./check-dependencies.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to check-prerequisites (we use the term "dependencies" to represent module/library dependencies)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of these prerequisites are only needed if you want to package (pack.sh) to multiple languages which is not what build.sh is doing. I am wondering if we should split those up to build-prereqs and pack-prereqs and only check for the packaging deps if you attempt packaging.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just saw that at least dotnet is required by the CLI unit tests...

So I think I am taking this comment back - let's just make sure the environment is fully ready.


# Prepare for build with references
/bin/bash scripts/generate-aggregate-tsconfig.sh > tsconfig.json

Expand Down
161 changes: 161 additions & 0 deletions check-dependencies.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

move to scripts/ please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

set -euo pipefail

# Testing with this to simulate different installed apps:
# docker run -it -v ~/Source/aws-cdk:/var/cdk ubuntu bash

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, docker is also a prerequisite. It's needed for @aws-cdk/aws-s3-assets tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check for docker >= 19.03

# Note: Don't use \d in regex, it doesn't work on Macs without GNU tools installed
# Use [0-9] instead

app=""
app_min=""
app_v=""

die() { echo "$*" 1>&2 ; exit 1; }
wrong_version() { echo "Found $app version $app_v. Install $app >= $app_min" 1>&2; exit 1; }

check_which() {
app=$1
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to locals:

Suggested change
app=$1
local app=$1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

min=$2

echo -e "Checking if $app is installed... \c"

w=$(which ${app}) || w=""

if [ -z $w ] || [ $w == "$app not found" ]
then
die "Missing dependency: $app. Install $app >= $min"
else
echo "Ok"
fi
}

# [Node.js >= 10.13.0]
# ⚠️ versions `13.0.0` to `13.6.0` are not supported due to compatibility issues with our dependencies.
app="node"
app_min="v10.13.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to store these into global variables?

This feels way more readable to me:

check_which node v10.13.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wrong_version function relies on them.

check_which $app $app_min
app_v=$(node --version)

# Check for version 10.*.* - 29.*.*
echo -e "Checking node version... \c"
if [ $(echo $app_v | grep -c -E "v[12][0-9]\.[0-9]+\.[0-9]+") -eq 1 ]
then
# Check for version 13.0 to 13.6
if [ $(echo $app_v | grep -c -E "v13\.[0-6]\.[0-9]+") -eq 1 ]
then
die "node versions 13.0.0 to 13.6.0 are not supported due to compatibility issues with our dependencies."
else
# Check for version < 10.13
if [ $(echo $app_v | grep -c -E "v10\.([0-9]|1[0-2])\.[0-9]+") -eq 1 ]
then
wrong_version
else
echo "Ok"
fi
fi
else
echo "Not 12"
wrong_version
fi

# TODO - Should we switch to JS here instead of bash since we know node is installed?
Copy link
Contributor

Choose a reason for hiding this comment

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

bash is awesome

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with Romain that we are making it harder to eventually modify the build to be friendly to Windows developers. The more we do in TS/JS, the less duplicate code we have to write in Powershell.

# We'd still need to shell out to check each app...

# [Yarn >= 1.19.1]
app="yarn"
app_min="1.19.1"
check_which $app $app_min
app_v=$(${app} --version)
echo -e "Checking yarn version... \c"
if [ $(echo $app_v | grep -c -E "1\.(19|2[0-9])\.[0-9]+") -eq 1 ]
then
echo "Ok"
else
wrong_version
fi

# [Java OpenJDK 8]

# Follow "More Info" on a new Mac when trying javac, install latest Oracle:
# javac 14.0.1

# apt install default-jdk on Ubuntu
# javac 11.0.7

# Install Coretto 8 from Amazon web site
# javac --version fails
# javac -version
# javac 1.8.0_252
# Old javac versions output version to stderr... why

app="javac"
app_min="1.8.0"
check_which $app $app_min
app_v=$(${app} -version 2>&1)
echo -e "Checking javac version... \c"
# 1.8
if [ $(echo $app_v | grep -c -E "1\.8\.[0-9].*") -eq 1 ]
then
echo "Ok"
else
# 11 or 14
if [ $(echo $app_v | grep -c -E "1[14]\.[0-9]\.[0-9].*") -eq 1 ]
then
echo "Ok"
else
wrong_version
fi
fi

# [Apache Maven]
app="mvn"
app_min="3.6.0"
check_which $app $app_min
app_v=$(${app} --version)
echo -e "Checking mvn version... \c"
if [ $(echo $app_v | grep -c -E "3\.[6789]\.[0-9].*") -eq 1 ]
then
echo "Ok"
else
wrong_version
fi

# [.NET Core SDK 3.1]
app="dotnet"
app_min="3.1.0"
check_which $app $app_min
app_v=$(${app} --version)
echo -e "Checking $app version... \c"
if [ $(echo $app_v | grep -c -E "3\.1\.[0-9].*") -eq 1 ]
then
echo "Ok"
else
wrong_version
fi

# [Python 3.6.5]
app="python3"
app_min="3.6.5"
check_which $app $app_min
app_v=$(${app} --version)
echo -e "Checking $app version... \c"
if [ $(echo $app_v | grep -c -E "3\.[6789]\.[0-9].*") -eq 1 ]
then
echo "Ok"
else
wrong_version
fi

# [Ruby 2.5.1]
app="ruby"
app_min="2.5.1"
check_which $app $app_min
app_v=$(${app} --version)
echo -e "Checking $app version... \c"
if [ $(echo $app_v | grep -c -E "2\.[56789]\.[0-9].*") -eq 1 ]
then
echo "Ok"
else
wrong_version
fi