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

Upgrade-16 cherry-picks round 2 #9625

Merged
merged 11 commits into from
Jul 1, 2024

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Jul 1, 2024

Rebase todo

# Branch fix-vow-include-vat-js-in-package-files-9607-
label base-fix-vow-include-vat-js-in-package-files-9607-
pick b6ffa6f09e fix(vow): include vat.js in package files
label fix-vow-include-vat-js-in-package-files-9607-
reset base-fix-vow-include-vat-js-in-package-files-9607-
merge -C a3826e91d9 fix-vow-include-vat-js-in-package-files-9607- # fix(vow): include vat.js in package files (#9607)

# Pull Request #9601
pick 6bc363b562 fix(cosmos): only allow snapshot export at latest height (#9601)

# Branch Force-xsnap-rebuild-9618-
label base-Force-xsnap-rebuild-9618-
pick 467435aeea fix(xsnap): force rebuild if build config changes
pick a22772e03e fix(agd): check xsnap was rebuilt
pick 2b53896d5d feat(xsnap): force rebuild if binary version mismatch
label Force-xsnap-rebuild-9618-
reset base-Force-xsnap-rebuild-9618-
merge -C 78b6fec067 Force-xsnap-rebuild-9618- # Force xsnap rebuild (#9618)

# Branch agd-enforce-Node-js-version-9623-
label base-agd-enforce-Node-js-version-9623-
#pick 4b35cafd57 revert fix: typescript-estree does not support Node.js LTS (#9619)
pick 5f01bef764 fix(agd): force own node.js version check
label agd-enforce-Node-js-version-9623-
reset base-agd-enforce-Node-js-version-9623-
merge -C 4f70e66a2f agd-enforce-Node-js-version-9623- # agd enforce Node.js version (#9623)

# Branch gibson-9623-followup
label base-gibson-9623-followup
pick 6102eb9149 fix: Disallow Node.js major version >20
label gibson-9623-followup
reset base-gibson-9623-followup
merge -C caaec0575a gibson-9623-followup # (upstream/master) fix: Disallow Node.js major version >20 (#9630)

mhofman and others added 7 commits July 1, 2024 02:27
## Problem

```
Error [ERR_MODULE_NOT_FOUND]: Cannot find module '.../node_modules/@agoric/vow/vat.js' imported from .../contract/test/test-contract.js
```

### Testing Considerations

This manual test passes with this PR but fails without it:

```console
~/projects/agoric-sdk/packages/vow$ yarn pack
~/projects/agoric-sdk/packages/vow$ tar tf *.tgz |grep vat
package/vat.js
```
closes: #9600

## Description
Check that an explicit height matches the latest height as exporting historical height is not supported by swing-store

### Security Considerations
None

### Scaling Considerations
None

### Documentation Considerations
Release notes should make clear limits of new support

### Testing Considerations
Manually tested explicit height

### Upgrade Considerations
Would be good to include in u16 if we cut a new rc1
closes: #9614
closes: #7012
refs: agoric-labs/xsnap-pub#49

## Description
Wire a mechanism to force rebuilds of xsnap when some build environments change, or if the xsnap binary is found to be out of date through the embedded xsnap package version.

Moves the `agd build` check to depend on a version check implemented by the xsnap package, which now only depends on the package version that is dynamically inserted instead of an explicit magic string that wasn't getting updated.

#7012 (comment) expressed concerns with relying on the xsnap package version, however:
- any changes to `xsnap-pub` or `moddable` submodules would result in a change to the checked in `build.env` file, which would be detected by `lerna` during the publish process. While a version bump would not apply for contributors of the sdk (aka not every xsnap change results in a version change), it will apply for anyone using published versions, even if the change is in submodules only.
- By having the version check look up the expected version from the `package.json` file directly, we avoid having to modify both  the `package.json` file and the check itself. Only the automatically generated single publish commit will trigger a forced rebuild, and satisfy the check on its own,
- At the same time we remove the dependency of `agd build` onto the the internal structure of xsnap.

### Security Considerations
Forces all validators to use the expected version of xsnap

### Scaling Considerations
Does a few more checks when building xsnap, and causes full rebuilds in some cases where they might not be strictly necessary.

### Documentation Considerations
Should all be transparent

### Testing Considerations
Manually tested by touching the xsnap package version, or reverting just the release binary to a previous copy (or deleting altogether). If the binary is meddled with like this, `agd build` checks will fail, but a manual `yarn build` will fix. I thought this was better than transparently forcing a rebuild in that abnormal situation.

### Upgrade Considerations
Smoother upgrades by validators
mhofman and others added 2 commits July 1, 2024 08:36
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 mhofman added the force:integration Force integration tests to run on PR label Jul 1, 2024
@mhofman mhofman marked this pull request as ready for review July 1, 2024 08:37
@mhofman mhofman force-pushed the mhofman/cherry-pick-u16-round-2 branch from 55fc53a to b0f1f66 Compare July 1, 2024 08:37
Copy link

cloudflare-workers-and-pages bot commented Jul 1, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: f91b395
Status: ✅  Deploy successful!
Preview URL: https://a6b460b6.agoric-sdk.pages.dev
Branch Preview URL: https://mhofman-cherry-pick-u16-roun.agoric-sdk.pages.dev

View logs

@gibson042 gibson042 self-requested a review July 1, 2024 16:15
gibson042 and others added 2 commits July 1, 2024 14:45
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).
@mhofman
Copy link
Member Author

mhofman commented Jul 1, 2024

@gibson042 I believe there is a late breaking change landing on master soon that needs inclusion in upgrade 16

@gibson042 gibson042 merged commit 5e17008 into dev-upgrade-16 Jul 1, 2024
75 checks passed
@gibson042 gibson042 deleted the mhofman/cherry-pick-u16-round-2 branch July 1, 2024 20:08
@gibson042
Copy link
Member

@gibson042 I believe there is a late breaking change landing on master soon that needs inclusion in upgrade 16

Yep, an expected derivative of #9620. It'll get its own dev-upgrade-16 PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants