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

refactor(wasm): modify turborepo config #780

Closed
wants to merge 20 commits into from
Closed

refactor(wasm): modify turborepo config #780

wants to merge 20 commits into from

Conversation

TalDerei
Copy link
Contributor

@TalDerei TalDerei commented Mar 18, 2024

references #676

The responsibility of downloading proving keys is moved to the wasm package. I further simplified the turborepo configuration defined in turbo.json, while still adhering to the existing dependency chain, by utilizing the precompile script hook provided by npm. This hook is automatically executed before the compile / dev / build scripts, ensuring the proving keys are available before proceeding with the compilation of the WASM package. The top-level scripts in package.json will call this precompile step before continuing with their respective execution. As such, we remove the unnecessary explicit download-key call in favor of this precompile step.

Additionally, per #434 (comment), we maintain a bin directory in the wasm package, and copy the bin directory inside the temporary dist directory in the extension package.

@TalDerei TalDerei self-assigned this Mar 18, 2024
@TalDerei TalDerei requested review from turbocrime, grod220 and jessepinho and removed request for turbocrime and grod220 March 20, 2024 16:04
@TalDerei
Copy link
Contributor Author

@turbocrime can you check why lint:rust is failing the CI? seems like wasm-pack doesn't seem to be available in the environment where the @penumbra-zone/wasm package's compile script is being executed?

.github/workflows/turbo-ci.yml Outdated Show resolved Hide resolved
apps/extension/src/utils/download-proving-keys.ts Outdated Show resolved Hide resolved
Comment on lines +115 to +123
patterns: [
{ from: 'public', to: '.' },
{
from: 'bin',
to: 'bin',
context: path.resolve(__dirname, '../../packages/wasm'),
noErrorOnMissing: true,
},
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc there is a more 'conventional' way to bundle assets from a dependency in webpack. i don't have an example on hand but i will see what i can find later

@@ -5,6 +5,7 @@
"license": "MIT",
"type": "module",
"scripts": {
"precompile": "tsx ../../apps/extension/src/utils/download-proving-keys.ts",
Copy link
Contributor

@turbocrime turbocrime Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent, i like how this leverages the pre/post auto-execute feature of npm. it might be more technically correct as a 'postcompile' or 'postbuild' script - or even 'pretest' 'prelint' or 'postinstall'

we might look at using more of these pre/post scripts for dependency management outside of turbo, which should be aware of them and manage cache appropriately - playwright-install before certain tests is another good candidate for this

turbo.json Outdated Show resolved Hide resolved
@turbocrime
Copy link
Contributor

Screenshot 2024-03-20 at 12 23 16
  1. it's preferable to use turbo cache for keys, to avoid redownloading keys at every CI step
  2. it's preferable to use turbo cache for compile output, to avoid setting up compile tools for every CI step

@TalDerei TalDerei requested a review from turbocrime March 21, 2024 18:08
@TalDerei
Copy link
Contributor Author

@turbocrime is this in a mergeable state? feel free to make any changes

@turbocrime
Copy link
Contributor

turbocrime commented Mar 28, 2024

probably the wasm package should contain the key download script, and expose it to other packages via the 'bin' field

@TalDerei TalDerei requested a review from turbocrime March 28, 2024 06:08
@turbocrime
Copy link
Contributor

#872

@TalDerei
Copy link
Contributor Author

closing in favor of #872

@TalDerei TalDerei closed this Apr 14, 2024
@TalDerei TalDerei deleted the config-pk branch April 14, 2024 14:28
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