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

Node version checks are inconsistent #9622

Open
mhofman opened this issue Jul 1, 2024 · 0 comments
Open

Node version checks are inconsistent #9622

mhofman opened this issue Jul 1, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@mhofman
Copy link
Member

mhofman commented Jul 1, 2024

Describe the bug

Our readme now mentions (#9295) that we officially support Node.js ^18.12 or ^20.9, aka the LTS versions.

We also integration test with the latest-ish of these versions. See #8920

However:

  • yarn install fails with Node.js >= 18.12 && < 18.18 because of an overeager dev dependency engines. fix: typescript-estree does not support Node.js LTS #9619 attempted to relax the restriction but the patch approach fails as it applies too late
  • SKIP_DOWNLOAD=false agd build will actually attempt to download Node.js 16 which is no longer supported at all. Furthermore, if SKIP_DOWNLOAD=true (the default), a failing Node.js check isn't fatal allowing to technically continue with untested versions

To Reproduce

agd build with Node.js 18.12

Expected behavior

It should build and agd start should work

Screenshots

yarn install v1.22.19
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
error @typescript-eslint/[email protected]: The engine "node" is incompatible with this module. Expected version "^18.18.0 || >=20.0.0". Got "18.15.0"
error Found incompatible module.
@mhofman mhofman added the bug Something isn't working label Jul 1, 2024
mergify bot added a commit that referenced this issue Jul 1, 2024
refs: #9622

## Description
Enforce the Node.js version in agd builder. Reverts #9619

This enforces LTS range, aka `^18.12` or `^20.9` through a regexp similar to the golang version check.

### Security Considerations
None

### Scaling Considerations
None

### Documentation Considerations
We'll need to update release notes for validators to use `agd build` instead of the former `yarn install` and `yarn build` as that may still fail. If they still chose to go that route, they're on their own.

### Testing Considerations
CI covers `agd build`

### Upgrade Considerations
Smooth the transition away from previously supported Node v16 in upgrade-15
mhofman pushed a commit that referenced this issue Jul 1, 2024
refs: #9622

## Description
Enforce the Node.js version in agd builder. Reverts #9619

This enforces LTS range, aka `^18.12` or `^20.9` through a regexp similar to the golang version check.

### Security Considerations
None

### Scaling Considerations
None

### Documentation Considerations
We'll need to update release notes for validators to use `agd build` instead of the former `yarn install` and `yarn build` as that may still fail. If they still chose to go that route, they're on their own.

### Testing Considerations
CI covers `agd build`

### Upgrade Considerations
Smooth the transition away from previously supported Node v16 in upgrade-15
mergify bot added a commit that referenced this issue Jul 1, 2024
refs: #9622

## Description
#9623 claimed to enforce Node.js versions ^18.12 or ^20.9, but due to a typo allows arbitrarily high versions to pass the check. This corrects it to enforce that the major version is exactly 18 or 20.

### Security Considerations
None.

### Scaling Considerations
n/a

### Documentation Considerations
Matches the [README](https://github.com/Agoric/agoric-sdk?tab=readme-ov-file#prerequisites).

### Testing Considerations
Checked by integration testing in CI.

### Upgrade Considerations
Will need an update for supporting Node.js v22 when that enters LTS (ref #9265).
gibson042 pushed a commit that referenced this issue Jul 1, 2024
refs: #9622

## Description
#9623 claimed to enforce Node.js versions ^18.12 or ^20.9, but due to a typo allows arbitrarily high versions to pass the check. This corrects it to enforce that the major version is exactly 18 or 20.

### Security Considerations
None.

### Scaling Considerations
n/a

### Documentation Considerations
Matches the [README](https://github.com/Agoric/agoric-sdk?tab=readme-ov-file#prerequisites).

### Testing Considerations
Checked by integration testing in CI.

### Upgrade Considerations
Will need an update for supporting Node.js v22 when that enters LTS (ref #9265).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant