-
Notifications
You must be signed in to change notification settings - Fork 212
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
ci: properly handle subdependencies for Endo branch override #7958
Conversation
@@ -130,6 +130,14 @@ test('create SES worker, save, restore, resume', async t => { | |||
* They are also sensitive to the XS code itself. | |||
*/ | |||
test('XS + SES snapshots are long-term deterministic', async t => { | |||
/* global globalThis */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. I did not know we could do this locally. Is it scoped to the block it appears in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, not scoped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It that case, I'd prefer it at the top of the file, since that lack of scoping is surprising and this misunderstanding fails unsafe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes let's move to the top
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, everything I understand LGTM. But I do not understand enough to approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preliminary review, but seems fine so far.
if [ ${{ steps.endo-branch.outputs.result }} == NOPE ]; then | ||
dirty=$(git status --porcelain) | ||
else | ||
dirty=$(git status --porcelain | egrep -ve '^ M (package\.json|yarn\.lock)$') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunate, but acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking this over. I'd like to pivot, and instead git restore package.json yarn.lock
after the custom Endo's yarn install
is done so that I can remove this special case.
@@ -130,6 +130,14 @@ test('create SES worker, save, restore, resume', async t => { | |||
* They are also sensitive to the XS code itself. | |||
*/ | |||
test('XS + SES snapshots are long-term deterministic', async t => { | |||
/* global globalThis */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes let's move to the top
@@ -130,6 +130,14 @@ test('create SES worker, save, restore, resume', async t => { | |||
* They are also sensitive to the XS code itself. | |||
*/ | |||
test('XS + SES snapshots are long-term deterministic', async t => { | |||
/* global globalThis */ | |||
const ENDO_BRANCH = globalThis.process?.env?.ENDO_BRANCH; | |||
if (ENDO_BRANCH && ENDO_BRANCH !== 'NOPE') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if instead we shouldn't run a yarn test --update-snapshots
(and ignore the clobbered snapshot artifacts in the porcelain check)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to make an assumption that this is the only SwingSet test using t.snapshot
. This approach seems less brittle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I see it, when we add a test that breaks under these endo changes, either we update the test, or we update the CI job to handle the test. The main problem is if there are golden test impacted by endo changes that do not use snapshots.
The CI config here could simply add --update-snapshot
to all yarn test
invocations if we're not worried about other snapshot based tests diverging in more significant ways.
scripts/replace-packages.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep this file around? I have found it useful for local testing too, where I don't care about the dependency resolution issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. (I forgot about Hyrum's Law.)
scripts/copy-packages-tgz.sh
Outdated
#! /bin/bash | ||
# Usage: copy-packages-tgz.sh <SRCDIR> | ||
# | ||
# This script copies package tarballs generated from the $SRCDIR workspace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to copy the tarballs at all? Aka can't the resolutions target the original package files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I'll make that change once I figure out what to name this script. :)
|
||
PACKAGEJSONHASH=$( | ||
jq --arg override "$override" '.resolutions *= ($override | fromjson)' package.json | | ||
git hash-object -w --stdin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting hack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this hack. It’s the best hack ever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big fan of the Shell, JQ, and Git software development stack. Now we just need an acronym for it that’s as compelling as LAMP.
SHJQIT, perchance? |
💩 |
ci: properly handle subdependencies for Endo branch override
ci: properly handle subdependencies for Endo branch override
ci: properly handle subdependencies for Endo branch override
closes: #7947
Description
The old implementation of
#endo-branch: XXX
hacked and slashed Endo tarballs into the checked out Agoric SDK, and in doing so, was extremely sensitive to conflicts in dependency versions between the two branches.This PR introduces a better way: use yarn
resolutions
to override the Endo versions directly with the tarballs, and let yarn sort out how the dependency version conflicts should be handled.As an aside, don't fail the SwingSet golden xsnap hash test when using an explicit Endo branch.
Security Considerations
n/a
Scaling Considerations
n/a
Documentation Considerations
n/a
Testing Considerations
Improves CI support.