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

Migrate to npm workspaces #1394

Merged
merged 3 commits into from
Jul 22, 2022
Merged

Migrate to npm workspaces #1394

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b0e9c0
Migrate to npm workspaces
CaptainIRS Jun 28, 2022
c9ac88b
Address review comments
CaptainIRS Jul 21, 2022
d53a25e
Address review comments
CaptainIRS Jul 22, 2022
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
2 changes: 1 addition & 1 deletion .build/benchmark-integration-test-direct.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ set -e
set -o pipefail

# Bootstrap the project again
npm i && npm run repoclean -- --yes && npm run bootstrap
npm ci

# Get the root directory of the caliper source
ROOT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )/.." && pwd )"
Expand Down
4 changes: 2 additions & 2 deletions .build/checks-and-unit-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ set -o pipefail
./scripts/check-package-names.sh

# Bootstrap the project again
npm i && npm run repoclean -- --yes && npm run bootstrap
npm ci

pushd ./packages/caliper-publish/
./publish.js version check
popd

# Run linting, license check and unit tests
npm test
npm run test --workspaces
2 changes: 1 addition & 1 deletion .build/publish-caliper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,6 @@ cp ./README.md ./packages/caliper-fabric/README.md
cp ./README.md ./packages/caliper-fisco-bcos/README.md

cd ./packages/caliper-publish/
npm i
npm ci
./publish.js npm
./publish.js docker --publish
10 changes: 10 additions & 0 deletions .github/workflows/integration-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}
- name: Install latest NPM
run: npm install -g npm@latest
CaptainIRS marked this conversation as resolved.
Show resolved Hide resolved
- name: Fabric Integration Test
run: .build/benchmark-integration-test-direct.sh
env:
Expand All @@ -33,6 +35,8 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}
- name: Install latest NPM
run: npm install -g npm@latest
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to fix our dependencies to specific versions so I think we should do the same here and fix to a specific version of npm. we could centralise this in a bootstrap script called by the benchmark-integration-test-direct.sh and then it's in a single place

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 think setting the version of npm in scripts like benchmark-integration-test-direct.sh or checks-and-unit-tests.sh would be problematic as since the docs mention developers to run those scripts before pushing a change, we might end up installing an unnecessary version of npm in the developer's machine when they run the script.

Copy link
Contributor

@davidkel davidkel Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The idea of the script would be to install a newer version only if required, otherwise not make any changes (but see comment below and ref discord chat). I would like to centralise for the build the version of npm required to a single point rather than replicating it across the different sut testing though.

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 have added a script for checking prerequisites. However I haven't added it to npm's preinstall script as there is a bug in npm (npm/cli#2660) due to which the preinstall script gets executed only after installing dependencies (whereas we want it to be executed before installing any dependency). I think the way to go is to update the contributor docs (https://hyperledger.github.io/caliper/v0.5.0/contributing/#bootstrapping-the-caliper-repository) to run the script before installing dependencies. I would like to know if there is any other way we could proceed.

- name: Ethereum Integration Test
run: .build/benchmark-integration-test-direct.sh
env:
Expand All @@ -50,6 +54,8 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}
- name: Install latest NPM
run: npm install -g npm@latest
- name: Besu Integration Test
run: .build/benchmark-integration-test-direct.sh
env:
Expand All @@ -67,6 +73,8 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}
- name: Install latest NPM
run: npm install -g npm@latest
CaptainIRS marked this conversation as resolved.
Show resolved Hide resolved
- name: FISCO BCOS Integration Test
run: .build/benchmark-integration-test-direct.sh
env:
Expand All @@ -84,6 +92,8 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}
- name: Install latest NPM
CaptainIRS marked this conversation as resolved.
Show resolved Hide resolved
run: npm install -g npm@latest
- name: Generator Integration Test
run: .build/benchmark-integration-test-direct.sh
env:
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/unit-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}
- name: Install latest NPM
CaptainIRS marked this conversation as resolved.
Show resolved Hide resolved
run: npm install -g npm@latest
- name: Check correct usage of Caliper package names
run: ./scripts/check-package-names.sh
- name: Install project dependencies
run: npm install
- name: Bootstrap lerna
run: npm run bootstrap
run: npm ci
- name: Check the version consistency of subpackages
run: ./packages/caliper-publish/publish.js version check
- name: Run unit tests
run: npm test
run: npm test --workspaces
2 changes: 0 additions & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ architecture.pptx
output.log
lerna-debug.log

**/package-lock.json

.idea/
**/node_modules/
**/log/
Expand Down
1 change: 1 addition & 0 deletions .npmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
engine-strict=true
1 change: 0 additions & 1 deletion CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ You can find the following main component types at the root of the repository:
* [.build/](.build/)
* [scripts/](scripts/)
* [azure-pipelines.yml](azure-pipelines.yml)
* [lerna.json](lerna.json)
* [package.json](package.json)
* Main code-based components of Caliper in the [packages/](packages/) directory

Expand Down
21 changes: 0 additions & 21 deletions lerna.json
View file
Open in desktop

This file was deleted.

Loading