chore: add prerequisite check before build #8929

ericzbeard · 2020-07-07T19:54:17Z

This PR is difficult to test, since it's checking for globally installed dependencies like Java, Dotnet, etc. I'm concerned mostly about the effects on our build process, since there may be regex patterns for valid installed versions that I missed.

Hopefully it will have a positive influence on adoption for new contributors by telling them exactly what they are missing before getting deep into a build and seeing confusing error messages.

Closes Issue #8847.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

eladb · 2020-07-08T11:40:03Z

build.sh

 /bin/bash ./git-secrets-scan.sh

+# Verify dependencies before starting the build
+/bin/bash ./check-dependencies.sh


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

eladb · 2020-07-08T11:40:10Z

check-dependencies.sh

@@ -0,0 +1,161 @@
+#!/bin/bash


move to scripts/ please

eladb · 2020-07-08T11:40:54Z

check-dependencies.sh

+    wrong_version
+fi
+
+# TODO - Should we switch to JS here instead of bash since we know node is installed?


bash is awesome

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.

eladb · 2020-07-08T11:41:38Z

check-dependencies.sh

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


Change to locals:

Suggested change

app=$1

local app=$1

eladb · 2020-07-08T11:42:44Z

check-dependencies.sh

+app="node"
+app_min="v10.13.0"


why do we need to store these into global variables?

This feels way more readable to me:

check_which node v10.13.0

The wrong_version function relies on them.

eladb · 2020-07-08T11:45:21Z

build.sh

 /bin/bash ./git-secrets-scan.sh

+# Verify dependencies before starting the build
+/bin/bash ./check-dependencies.sh


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.

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.

RomainMuller

I'd rather we stop introducing more bash and instead code this up in Javascript (or TypeScript with ts-node if we are okay with assuming npx is there -- aka a sufficiently recent node is present)

eladb · 2020-07-08T15:49:39Z

I'd rather we stop introducing more bash and instead code this up in Javascript (or TypeScript with ts-node if we are okay with assuming npx is there -- aka a sufficiently recent node is present)

Why? We don't have a goal or need to support non-Linux development environments. If we did, we'd have to make a major refactor to our build system. I wouldn't attempt to write this script as a portable scripts.

ericzbeard · 2020-07-08T15:55:45Z

I'd rather we stop introducing more bash and instead code this up in Javascript (or TypeScript with ts-node if we are okay with assuming npx is there -- aka a sufficiently recent node is present)

Why? We don't have a goal or need to support non-Linux development environments. If we did, we'd have to make a major refactor to our build system. I wouldn't attempt to write this script as a portable scripts.

Doesn't it become a lot more practical once we get to monocdk? I think we should consider this as a goal for next year, if not Q4.

eladb · 2020-07-08T17:40:23Z

check-dependencies.sh

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


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

Added a check for docker >= 19.03

Pull request has been modified.

eladb · 2020-07-09T07:11:52Z

scripts/check-prerequisites.sh

+    wrong_version
+fi
+
+# [Docker 19.03.x]


We should also check that docker daemon is running and accessible (basically run docker ps or docker info I think)

eladb · 2020-07-09T07:14:13Z

scripts/check-prerequisites.sh

+    wrong_version
+fi
+
+# [Docker 19.03.x]


I think we can be much more relaxed about the docker version or otherwise we will constantly have to change this. Docker hasn’t had breaking CLI changes in a couple of years to my knowledge.

I’d just check that docker is available and docker daemon is accessible.

eladb · 2020-07-09T07:15:33Z

scripts/check-prerequisites.sh

+    wrong_version
+fi
+
+# [Ruby 2.5.1]


Please update these comments to reflect the semver description.

For example, here you are checking ^2.5.1 or >= 2.5.1 <3.0.0, right?

Pull request has been modified.

ericzbeard · 2020-07-11T23:54:01Z

@eladb I don't have access to the build logs to see why this is failing.

ericzbeard · 2020-07-13T16:16:27Z

@eladb The error seems unrelated to my changes:
npm ERR! code ETARGET
npm ERR! notarget No matching version found for @aws-cdk/pipelines@1.50.0.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

eladb · 2020-07-13T18:06:49Z

@eladb The error seems unrelated to my changes:
npm ERR! code ETARGET
npm ERR! notarget No matching version found for @aws-cdk/pipelines@1.50.0.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

Probably requires diving deeper. Did you manage to build this locally?

@rix0rrr might be able to help out given this seems related to CDK pipelines...

ericzbeard · 2020-07-13T22:03:17Z

@eladb I think it was a transient thing that happened on master around the time I pushed my last change. How can I force this to try again? Push a whitespace commit?

eladb · 2020-07-13T22:15:48Z

Update your branch from master

mergify · 2020-07-14T17:38:05Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-07-14T18:39:09Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 3ea4357
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-07-14T18:39:34Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

This PR is difficult to test, since it's checking for globally installed dependencies like Java, Dotnet, etc. I'm concerned mostly about the effects on our build process, since there may be regex patterns for valid installed versions that I missed. Hopefully it will have a positive influence on adoption for new contributors by telling them exactly what they are missing before getting deep into a build and seeing confusing error messages. Closes Issue aws#8847. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

ericzbeard and others added 7 commits May 26, 2020 09:40

docs(contributing): Add note on global npm installs, change OpenJDK URL

757290e

Merge branch 'master' into master

3a4b46b

Merge remote-tracking branch 'upstream/master'

afe2119

Merge remote-tracking branch 'upstream/master'

4aa3f0d

Merge remote-tracking branch 'upstream/master'

3c1ee3b

Check dependencies before building

46dceca

Merge remote-tracking branch 'upstream/master' into issue_8847

5d471ae

ericzbeard requested review from RomainMuller and eladb July 7, 2020 19:54

ericzbeard changed the title ~~feat(core) check dependencies before building~~ chore(core) check dependencies before building Jul 7, 2020

SomayaB assigned eladb Jul 7, 2020

eladb changed the title ~~chore(core) check dependencies before building~~ chore: add prerequisite check before build Jul 8, 2020

eladb previously requested changes Jul 8, 2020

View reviewed changes

RomainMuller previously requested changes Jul 8, 2020

View reviewed changes

eladb reviewed Jul 8, 2020

View reviewed changes

ericzbeard added 5 commits July 8, 2020 11:21

Merge remote-tracking branch 'upstream/master' into issue_8847

402ef5f

Move script and add docker

a61b65f

Merge remote-tracking branch 'upstream/master' into issue_8847

0d4e1a8

Add docker prerequiresite to contributing guide

777adab

Move check-prerequisites.sh

3586802

eladb previously requested changes Jul 9, 2020

View reviewed changes

Minor changes to check-prerequisites.sh

e529db7

Merge remote-tracking branch 'upstream/master' into issue_8847

14c14a4

eladb approved these changes Jul 14, 2020

View reviewed changes

Merge branch 'master' into issue_8847

3ea4357

mergify bot merged commit a3008fc into aws:master Jul 14, 2020

ericzbeard mentioned this pull request Jul 15, 2020

Improve error messages for missing build dependencies #8847

Closed

kellertk mentioned this pull request Sep 18, 2020

[core] "Full docker build" no longer works #10438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add prerequisite check before build #8929

chore: add prerequisite check before build #8929

ericzbeard commented Jul 7, 2020 •

edited

Loading

eladb Jul 8, 2020

ericzbeard Jul 8, 2020

eladb Jul 8, 2020

ericzbeard Jul 8, 2020

eladb Jul 8, 2020

ericzbeard Jul 8, 2020

eladb Jul 8, 2020

ericzbeard Jul 8, 2020

eladb Jul 8, 2020

ericzbeard Jul 8, 2020

eladb Jul 8, 2020

eladb Jul 8, 2020

RomainMuller left a comment

eladb commented Jul 8, 2020

ericzbeard commented Jul 8, 2020

eladb Jul 8, 2020

ericzbeard Jul 8, 2020

eladb Jul 9, 2020

eladb Jul 9, 2020

eladb Jul 9, 2020

ericzbeard commented Jul 11, 2020

ericzbeard commented Jul 13, 2020

eladb commented Jul 13, 2020

ericzbeard commented Jul 13, 2020

eladb commented Jul 13, 2020

mergify bot commented Jul 14, 2020

aws-cdk-automation commented Jul 14, 2020

mergify bot commented Jul 14, 2020


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

		app="node"
		app_min="v10.13.0"

chore: add prerequisite check before build #8929

chore: add prerequisite check before build #8929

Conversation

ericzbeard commented Jul 7, 2020 • edited Loading

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

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

RomainMuller left a comment

Choose a reason for hiding this comment

eladb commented Jul 8, 2020

ericzbeard commented Jul 8, 2020

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

ericzbeard commented Jul 11, 2020

ericzbeard commented Jul 13, 2020

eladb commented Jul 13, 2020

ericzbeard commented Jul 13, 2020

eladb commented Jul 13, 2020

mergify bot commented Jul 14, 2020

aws-cdk-automation commented Jul 14, 2020

AWS CodeBuild CI Report

mergify bot commented Jul 14, 2020

ericzbeard commented Jul 7, 2020 •

edited

Loading