NeurodataWithoutBorders / nwb-guide

NWB GUIDE is a desktop app that provides a no-code user interface for converting neurophysiology data to NWB.
https://nwb-guide.readthedocs.io/
MIT License
22 stars 4 forks source link

Try using node installed from conda #902

Closed rly closed 1 month ago

rly commented 2 months ago

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:

rly commented 2 months ago

GitHub Actions now passes npm info run electron@26.6.9 postinstall but stalls a few steps later on npm info run esbuild@0.20.2 postinstall

CodyCBakerPhD commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

CodyCBakerPhD commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@rly Good to close?