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

#endo-branch: mechanism incomplete or broken #9232

Closed
erights opened this issue Apr 14, 2024 · 3 comments · Fixed by #9237
Closed

#endo-branch: mechanism incomplete or broken #9232

erights opened this issue Apr 14, 2024 · 3 comments · Fixed by #9237
Assignees
@michaelfig
Labels
bug Something isn't working

Comments

@erights
Copy link
Member

erights commented Apr 14, 2024

At https://github.com/Agoric/agoric-sdk/actions/runs/8680969004/job/23802757406?pr=6432#step:6:173 we see

Error#1: 'mustCompress' is not exported by ../../node_modules/@endo/patterns/index.js, imported by ../swingset-liveslots/src/virtualObjectManager.js

from #6432 , whose PR comment starts with

#endo-branch: markm-pattern-based-compression-2

That is the branch of endojs/endo#1584 , which does export mustCompress at

https://github.com/endojs/endo/blob/4ce652632397059410befb4f637c94cd83609198/packages/patterns/index.js#L76

So apparently, the @endo/patterns imported by liveslots is not the @ndo/patterns from that fork.

@michaelfig , ok to assign to you?
@erights erights added the bug Something isn't working label Apr 14, 2024
@erights erights assigned erights and michaelfig and unassigned erights Apr 14, 2024
@erights erights mentioned this issue Apr 14, 2024
Draft
@michaelfig
Copy link
Member

ok to assign to you?

Yes, definitely! For prioritisation, could you let me know which upgrade this is blocking? I'm mostly working on things that are blocking agoric-upgrade-16 right now.

@michaelfig michaelfig mentioned this issue Apr 15, 2024
Merged
@michaelfig
Copy link
Member

Turned out to be much easier to diagnose than I expected. See #9237.

@mhofman
Copy link
Member

mhofman commented Apr 19, 2024

FWIW, the endo-branch mechanism is also incomplete as it doesn't effect docker based tests: #8938

@mergify mergify bot closed this as completed in #9237 Apr 19, 2024
mergify bot added a commit that referenced this issue Apr 19, 2024
@mergify
ci(restore-node): preserve package.json timestamp (#9237)
a55bc1e 
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

<!-- Most PRs should close a specific Issue. All PRs should at least
reference one or more Issues. Edit and/or delete the following lines as
appropriate (note: you don't need both `refs` and `closes` for the same
one): -->

closes: #9232
refs: #6432

## Description

The `restore-node` Github action was updating the timestamp on
`package.json` if there was an `#endo-branch: XXX` setting on the PR.
This caused `agd-builder.sh` to rebuild the cached build artifacts
unnecessarily, including using incorrect `@endo/*` package versions (the
ones in the PR's `package.json`s, not the ones from branch `XXX`).

Now the `package.json` is copied (preserving its timestamp), and moved
back into place after installing any replaced packages.

<!-- Add a description of the changes that this PR introduces and the
files that
are the most critical to review.
-->

### Security Considerations

n/a

<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations

With this PR, shorter CI runs.

<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations

n/a

<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

### Testing Considerations

n/a

<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

### Upgrade Considerations

n/a

<!-- What aspects of this PR are relevant to upgrading live production
systems, and how should they be addressed? -->
This was referenced Aug 6, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@michaelfig michaelfig

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ci(restore-node): preserve package.json timestamp Agoric/agoric-sdk
3 participants
@mhofman @erights @michaelfig