-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix Node binary incompatibility #155
base: main
Are you sure you want to change the base?
Conversation
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 only have intel CPUs, so I'm not in a good position to verify this.
@@ -35,7 +35,11 @@ const fixupProposal = (proposal: ProposalInfo) => { | |||
|
|||
// refresh install | |||
execSync('rm -rf node_modules', { cwd: proposalPath }); | |||
execSync('yarn install', { cwd: proposalPath }); | |||
// install to update yarn.lock and get importable typed modules but | |||
// skip building because the proposal never runs on the local filesystem. |
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.
my mental model is missing something...
What provides the binaries when the proposal runs?
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.
node_modules
in the Docker image. It's already there. I had hoped that the mount would only be an overlay, but that was wrong. With mounting there's no way around running yarn install
inside the container. I'll try to automate that so the developer doesn't have to remember/know.
I replaced one
I then ran details
|
If I do
|
closes: #151
Test plan
I ran the updated doctor locally and confirmed there's no
.node
file.I haven't yet tested with
test --debug
because my env is running into a different problem missing--no-validate
. @Chris-Hibbert can you confirm this fixes #151? (e.g. copy this changea3p-integration/node_modules/@agoric/synthetic-chain/dist/cli/cli.js
then runyarn doctor
and thenyarn test --debug -m upgrade-next
)