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.

Sign up for GitHub

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

Jump to bottom

chore: build.sh script unnecessarily requires lerna to be globally installed #24217

Merged
merged 7 commits into from
Apr 12, 2023
Merged

chore: build.sh script unnecessarily requires lerna to be globally installed #24217

merged 7 commits into from
Apr 12, 2023

Conversation

timmattison
Copy link
Contributor

Either lerna needs to be installed as a dependency or run through npx. Otherwise build.sh fails in the middle of a build on a system without it installed.

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

@timmattison
Fix missing lerna dependency
bb36d37 
Either lerna needs to be installed as a dependency or run through npx. Otherwise build.sh fails in the middle of a build on a system without it installed.
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Feb 17, 2023
@gitpod-io
Copy link

gitpod-io bot commented Feb 17, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team February 17, 2023 17:13
@github-actions github-actions bot added the p2 label Feb 17, 2023
aws-cdk-automation
aws-cdk-automation previously requested changes Feb 17, 2023
View reviewed changes
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@timmattison timmattison changed the title Fix missing lerna dependency fix: Missing lerna dependency Feb 17, 2023
@timmattison
Copy link
Contributor Author

Exemption Request

My changes un-break the build. I'm not sure how I would write a test for it.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Feb 17, 2023
@timmattison
Copy link
Contributor Author

Exemption Request

My changes un-break the build. I'm not sure how I would write a test for it.

@madeline-k madeline-k mentioned this pull request Mar 9, 2023
Closed
@madeline-k madeline-k changed the title fix: Missing lerna dependency chore: build.sh script requires lerna to be globally installed Mar 9, 2023
@madeline-k madeline-k changed the title chore: build.sh script requires lerna to be globally installed chore: build.sh script unnecessarily requires lerna to be globally installed Mar 9, 2023
madeline-k
madeline-k previously approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@madeline-k madeline-k left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @timmattison! Changing this to a "chore" instead of a "fix" because it's developer/contributor facing and not customer facing. (Anything labeled as "fix" or "feat" ends up in the changelog and release notes, which is not necessary here.)

@aws-cdk-automation aws-cdk-automation dismissed their stale review March 9, 2023 22:13

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@mergify
Copy link
Contributor

mergify bot commented Mar 9, 2023

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

@mergify mergify bot dismissed madeline-k’s stale review March 9, 2023 22:21

Pull request has been modified.

mrgrain
mrgrain previously requested changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@mrgrain mrgrain left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@timmattison @madeline-k Are you sure that's what's going on? Because in line 38 the scripts adds node_modules/.bin to the path and in line 48 it does a yarn install which would install lerna at the right version.

And this does work on my machine (I know, I know) and in CI. So something else must be going on.

Either way, if we want to use npx it would at least have to be qualified to the correct version npx lerna@^4.0.0

PS: @madeline-k feel free to just dismiss my review if I'm blocking something =)

@madeline-k
Copy link
Contributor

@timmattison @madeline-k Are you sure that's what's going on? Because in line 38 the scripts adds node_modules/.bin to the path and in line 48 it does a yarn install which would install lerna at the right version.

And this does work on my machine (I know, I know) and in CI. So something else must be going on.

Either way, if we want to use npx it would at least have to be qualified to the correct version npx lerna@^4.0.0

PS: @madeline-k feel free to just dismiss my review if I'm blocking something =)

@mrgrain You are completely correct, the script does install lerna. And the goal of this change is to actually use the version of lerna installed locally in the projects dependencies. The problem now is that running the lerna command will run whatever version is installed globally, if there is one. I think the original title of the PR was a bit misleading. The lerna dependency is not missing, we just need the script to execute it from the projects dependencies.

(The npx command will run requested packages from the local project dependencies first, before trying to get it from the npm cache or the internet. See docs.)

madeline-k
madeline-k previously approved these changes Mar 17, 2023
View reviewed changes
@madeline-k madeline-k dismissed mrgrain’s stale review March 17, 2023 00:05

Addressed in comments

@mergify
Copy link
Contributor

mergify bot commented Mar 17, 2023

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

@madeline-k
Copy link
Contributor

@mrgrain, dismissing your review. We can always revert this if I am incorrect on something, but I think the change is good as is.

@mergify mergify bot dismissed madeline-k’s stale review March 17, 2023 00:07

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

3 similar comments
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

4 similar comments
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

TheRealAmazonKendra
TheRealAmazonKendra approved these changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Looks like our automations weren't working correctly here when previously approved.

@TheRealAmazonKendra TheRealAmazonKendra added the pr/do-not-merge This PR should not be merged at this time. label Apr 3, 2023
TheRealAmazonKendra
TheRealAmazonKendra previously requested changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Actually, it looks like a piece of the command got removed with this change. We want the concurrency setting left in there.

@TheRealAmazonKendra TheRealAmazonKendra removed pr/do-not-merge This PR should not be merged at this time. pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Apr 3, 2023
madeline-k
madeline-k suggested changes Apr 11, 2023
View reviewed changes
build.sh Outdated Show resolved Hide resolved
madeline-k
madeline-k previously approved these changes Apr 11, 2023
View reviewed changes
@mergify mergify bot dismissed TheRealAmazonKendra’s stale review April 11, 2023 22:27

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Apr 11, 2023

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

@mergify mergify bot dismissed madeline-k’s stale review April 12, 2023 21:45

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Apr 12, 2023

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

@mergify mergify bot merged commit 331622b into aws:main Apr 12, 2023
@mergify
Copy link
Contributor

mergify bot commented Apr 12, 2023

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

douglasnaphas added a commit to douglasnaphas/aws-cdk that referenced this pull request Jun 9, 2023
@douglasnaphas
Remove npm bin from build.sh
eb02701 
`npm bin` was [removed in npm 9](npm/cli#5459).
`npx` or `npm exec` should be used instead. `build.sh` already uses `npm
exec`, since aws#24217.
douglasnaphas added a commit to douglasnaphas/aws-cdk that referenced this pull request Jun 9, 2023
@douglasnaphas
Remove npm bin from scripts/gen.sh.
74bde33 
`npm bin` was [removed in npm 9](npm/cli#5459).
`npx` or `npm exec` should be used instead. [This
comment](aws#24217 (comment))
on a similar PR, and the discussion on the [PR in npm removing `npm
bin`](npm/cli#5459) explain why `npx lerna` is
preferable.
@douglasnaphas douglasnaphas mentioned this pull request Jun 9, 2023
Merged
mergify bot pushed a commit that referenced this pull request Jun 12, 2023
@douglasnaphas
chore: Remove npm bin from scripts (#25913)
12fe60e 
This change removes the expression `export PATH=$(npm bin):$PATH`, which had formerly been used in scripts to ensure `node_modules` is in `PATH`.

`npm bin` was [removed in npm 9](npm/cli#5459). `npm exec` or `npx` should be used instead. `build.sh` already uses `npx`. This change revises `scripts/gen.sh` to use `npx` as well.

Prior to this change, within shells executing `build.sh` or `scripts/gen.sh`, `PATH` actually contains error text if npm 9+ is used.

```
~/repos/aws-cdk $ docker run --rm -v $PWD:$PWD -w $PWD node:hydrogen-alpine sh -c 'node --version && npm --version && export PATH=$(npm bin):$PATH && echo $PATH' # output when npm bin is unavailable
v18.16.0
9.5.1
Unknown command: "bin" To see a list of supported npm commands, run: npm help:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

~/repos/aws-cdk $ docker run --rm -v $PWD:$PWD -w $PWD node:gallium-alpine sh -c 'node --version && npm --version && export PATH=$(npm bin):$PATH && echo $PATH' # output when npm bin is available
v16.20.0
8.19.4
/Users/douglasnaphas/repos/aws-cdk/node_modules/.bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
```

It didn't make `build.sh` fail, because `lerna` has been run via `npx` since #24217, and `build.sh` doesn't need anything from `node_modules` to be added to `PATH`. `export PATH=$(npm bin):$PATH` succeeds even though `npm bin` fails, per `export`'s normal behavior.

Prior to this change, `scripts/gen.sh` failed with

```
./scripts/gen.sh: line 18: lerna: command not found
```

when I ran it. After this change, `scripts/gen.sh` ran successfully.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@filletofish filletofish mentioned this pull request Jul 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheRealAmazonKendra TheRealAmazonKendra TheRealAmazonKendra approved these changes

@madeline-k madeline-k madeline-k left review comments

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

@mrgrain mrgrain mrgrain left review comments

Assignees
No one assigned
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@timmattison @madeline-k @aws-cdk-automation @mrgrain @TheRealAmazonKendra @Naumel