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

Try using node installed from conda #902

Closed
wants to merge 13 commits into from
Closed

Try using node installed from conda #902

wants to merge 13 commits into from

Conversation

rly
Copy link
Collaborator

@rly rly commented Jul 29, 2024

the workflows first use conda to install node 18. then they install node 20. this might result in conflicts. https://stackoverflow.com/questions/41524903/why-is-npm-install-really-slow

testing:

  • do not install node 20.
  • install node 20 from conda instead of node 18
  • try later: upgrade electron from 26.2.2 to 31.3.0

@rly
Copy link
Collaborator Author

rly commented Jul 29, 2024

GitHub Actions now passes npm info run [email protected] postinstall but stalls a few steps later on npm info run [email protected] postinstall

@CodyCBakerPhD
Copy link
Collaborator

OK, I'll let you play around with it a bit more

If we can't get it working within a few days though, I think we should just go ahead and cut a new release - would you be able to build and sign the mac ones yourself in that event?

@rly
Copy link
Collaborator Author

rly commented Jul 29, 2024

If we can't get it working within a few days though, I think we should just go ahead and cut a new release - would you be able to build and sign the mac ones yourself in that event?

Yes, can do.

@rly
Copy link
Collaborator Author

rly commented Jul 30, 2024

I think I figured it out. The issue is due to puppeteer. I see two options to resolve this:

Option 1: update Puppeteer to v22 (ref)

Option 2: use PUPPETEER_DOWNLOAD_BASE_URL=https://storage.googleapis.com/chrome-for-testing-public npm ci (ref)

I think we might as well update in Option 1.

Second question: I updated node, npm, electron packages, and vite. I looked through the electron release notes and saw nothing breaking that affects GUIDE. Vite 5 says The CJS build of Vite's Node API is deprecated. See https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.. So I renamed vite.config.js and electron.vite.config.js to have .mjs extensions and vite was happy. The single session tutorial works. Eventually the current versions will become unsupported in the ecosystem, so I think if everything still works, we should update. What do you think @CodyCBakerPhD ?

@rly rly requested a review from CodyCBakerPhD July 30, 2024 00:51
@rly
Copy link
Collaborator Author

rly commented Jul 30, 2024

Tests are failing. I spoke too soon. Will investigate tomorrow.

@CodyCBakerPhD
Copy link
Collaborator

Could be random failures, sometimes retries are necessary

So strange that some platforms do complete just fine. Have you tested it completely locally using a completely fresh node install?

@CodyCBakerPhD
Copy link
Collaborator

Although making that many updates to the package.json file was also something we were wary about since apparently the behavior of components/functions we use from those sources do seem to change quite a bit across versions

@CodyCBakerPhD
Copy link
Collaborator

@rly Another thought was that we could break a part the pieces of the testing suite even more across the CI, and have the base level tests run pytest/vitetest, and have end to end tests through puppeteer as an entirely separate workflow (then that would be the only one have issues)

On top of that, we could just do a targetted install of puppeteer in its own workflow rather than as a part of the broader package

Can discuss more at meeting today - WDYT?

@rly
Copy link
Collaborator Author

rly commented Aug 5, 2024

Just to recap our meeting:

  1. "break a part the pieces of the testing suite even more across the CI, and have the base level tests run pytest/vitetest, and have end to end tests through puppeteer as an entirely separate workflow (then that would be the only one have issues)

On top of that, we could just do a targetted install of puppeteer in its own workflow rather than as a part of the broader package" -- I agree. I'll work on that.

  1. Limiting updates to package.json. I agree. We should not update these in this PR since it should not be necessary here. We also decided we should update these on a regular basis, such as annually, when we have time to do lots of testing. This will make it easier to update when updating is required for some bug fix.

I'll get to these around the end of the week.

@CodyCBakerPhD
Copy link
Collaborator

@rly Good to close?

@rly rly closed this Aug 14, 2024
@rly rly deleted the mac_build_testing branch August 14, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants