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

build: consistent build/clean scripts #633

Merged
merged 37 commits into from
Apr 24, 2023
Merged

build: consistent build/clean scripts #633

merged 37 commits into from
Apr 24, 2023

Conversation

holic
Copy link
Member

@holic holic commented Apr 17, 2023

Fixes #615

This will help us create a dependency graph of scripts, better caching, etc. (See #620)

The format I took with these build steps is to keep them focused on their output (e.g. build:js is for TS -> JS). These are also in execution order.

  • build:go runs golang build steps
  • build:protobuf runs codegen for protobuf
  • build:mud runs mud-specific codegen
  • build:abi runs forge build --extra-output-files abi --out abi --skip test script for abi files to be used by typechain
    • this is separate from a regular forge build in that it 1) also generates abi files, for better imports 2) outputs to a separate directory (for easier typechain targeting) and 3) skips generating files for tests/scripts to avoid cluttering typechain
  • build:typechain runs typechain to generate typed ethers contracts/interfaces
  • build:wasm runs asc (just for noise package)
  • build:js runs tsup or rollup (see Consolidate JS build tooling #612)

Each build:* script also has a corresponding clean:* script, where applicable. Each package also has a corresponding build and clean that is a combo of any of the above (depending on what the package is doing). I expect build will only be run locally, and nx will use the steps themselves (to resolve dependent steps and run in parallel) (see #620)

Some other things to note:

  • I moved one-off scripts (store's tightcoder, cli's test tables) out of the build pipeline, since they should only be run on an as-needed basis. These should be validated as up-to-date by a future lint step (like we do for gas reports).
  • clean:abi uses rimraf abi instead of forge clean because the output directory is separate from the one configured by foundry.toml (used by tests, scripts)

TODO:

  • packages
    • root package
    • cli
    • common-codegen
    • config
    • create-mud
    • ecs-browser
    • network
    • noise
    • phaserx
    • react
    • recs
    • schema-type
    • solecs
    • std-client
    • std-contracts
    • store
    • utils
    • world
    • docs
  • fix circular dependencies (Move tablegen to Store and worldgen to World to resolve circular dependencies #616) to fix build
  • after circ deps fix, update ../cli/dist/mud.js to mud with proper dep
    • if we still can't, may need to add back in chmod +x step
  • update templates to match?
  • move store's custom codegen into a one-off script and out of the build/test pipeline?
  • do we need a build:forge for general building of solidity (i.e. tests, scripts) or let it get handled by e.g. forge test? do we need a clean:forge for out dir?
  • rethink prepack/postpack so that pack failures don't leave bad files around, or remove the need for it entirely
  • output typechain to ./typechain instead of default ./types/ethers-contracts?
  • do solecs, std-contracts still need to do the out -> exports.txt -> abi conversion (prev dist task)?
  • can we combine build:codegen and build:mudcodegen steps? I mostly separated because 1) some mud codegen depended on local cli to be build (thus diff dep tree) and 2) to separate from other types of codegen (e.g. typechain), maybe we move it all into a build:mud step?
  • std-client can't clean up its mud definitions because it has a .test-d.ts in the same dir, consider relocating?
  • remove build:abi step from schema-type? not currently in use
  • consider foundry profiles for abi output vs regular out
    • these are separated because tests/scripts need the full compiled output and abi output only builds what is needed to export abis for downstream use
  • make sure docs are not part of the build pipeline

@holic holic mentioned this pull request Apr 17, 2023
4 tasks
@holic holic force-pushed the holic/build-scripts branch from 309f8e0 to 413c3f5 Compare April 17, 2023 15:36
@@ -8,16 +8,21 @@
"directory": "packages/std-client"
},
"license": "MIT",
"type": "module",
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this so I could move codegen.mts to codegen.ts and align this with the rest of our packages. I ran templates/minimal with this and it seemed to work just fine.

"dev": "vite build --watch",
"generate-test-tables": "tsx ./scripts/generate-test-tables.ts",
Copy link
Member Author

Choose a reason for hiding this comment

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

This looked more like a one-off script so I moved it out of the build path (doesn't need to be regenerated each time and is expected to be run locally and changes committed).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, though it's easy to forget to run this (and this one changes often, as it's meant to test any changes to tablegen), which is why it was part of test, even though it doesn't really fit there too well.
If not part of test, it would be nice to have it somewhere else in CI at least

Copy link
Member Author

Choose a reason for hiding this comment

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

added back into test script for now, will refactor in a follow up as part of #637

"build:typechain": "typechain --target ethers-v5 'abi/**/*.sol/!(*.abi).json'",
"clean": "pnpm run clean:abi && pnpm run clean:typechain && pnpm run clean:js",
"clean:abi": "rimraf abi",
"clean:js": "rimraf dist",
"clean:typechain": "rimraf types",
"docs": "rimraf API && hardhat docgen",
"gas-report": "../cli/dist/mud.js gas-report --path test/* --path test/**/* --save gas-report.json",
"generate-tightcoder": "tsx ./scripts/generate-tightcoder.ts && prettier --write '**/tightcoder/*.sol'",
Copy link
Member Author

Choose a reason for hiding this comment

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

This looked more like a one-off script so I moved it out of the build path (doesn't need to be regenerated each time and is expected to be run locally and changes committed).

Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't really change/matter, but if we have some localgen script for generate-test-tables, tightcoder could also use it

"test": "pnpm run test:solidity && pnpm run test:typescript",
"test:solidity": "forge test",
"test:typescript": "tsc --noEmit"
"test": "tsc --noEmit && forge test"
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm planning to come back to this in #637

@@ -1,3 +1,2 @@
packages:
- packages/*
- docs
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this out for now to avoid running build on docs when it doesn't depend on anything in the workspace.

Instead, we may want to treat this as a separate app and adjust our docs scripts throughout to generate files into this docs/.

"build:js": "vite build",
"build:tightcoder": "tsx ./ts/scripts/codegenTightcoder.ts && prettier --write '**/*.sol'",
"build:mud": "tsx ./ts/scripts/tablegen.ts",
"build:typechain": "typechain --target ethers-v5 'abi/**/*.sol/!(*.abi).json'",
"clean": "pnpm run clean:abi && pnpm run clean:typechain && pnpm run clean:js",
"clean:abi": "rimraf abi",
"clean:js": "rimraf dist",
"clean:typechain": "rimraf types",
"docs": "rimraf API && hardhat docgen",
"gas-report": "../cli/dist/mud.js gas-report --path test/* --path test/**/* --save gas-report.json",
Copy link
Member Author

Choose a reason for hiding this comment

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

😭 Still need CLI here, but at least this isn't in the build path. I tried to add CLI as a dep, but that creates circular deps again. I also tried to add CLI as a dep in the root package, but there are no bins to link when it attempts to install from the workspace.

We could use whatever npm version is published, but that seems not ideal if we ever want to test changes to the gas report and want to use the local/workspace version.

We could also move the gas report out to common?

Copy link
Contributor

Choose a reason for hiding this comment

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

the tablegen treatment for gas-report would make sense imo, except yeah it'd go to common rather than store/world. And cli just gets a wrapper over the imported script

Copy link
Member Author

Choose a reason for hiding this comment

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

added an issue to track this here: #667

@holic holic marked this pull request as ready for review April 20, 2023 23:16
@holic holic requested a review from alvrs as a code owner April 20, 2023 23:16
@holic
Copy link
Member Author

holic commented Apr 20, 2023

Ready for review! I left a few non-blocking TODOs undone, so we can talk about em and decide if we should create follow up issues/PRs.

In terms of the build scripts defined above (and in this PR), is it worth adding a little CI script to validate that no new types of build:* scripts are added (without also extending the CI check)? This would at least make it a little more clear about the intent behind these.

dk1a
dk1a previously approved these changes Apr 21, 2023
Copy link
Contributor

@dk1a dk1a left a comment

Choose a reason for hiding this comment

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

lgtm, though I would prefer to not put off automating generate-test-tables in some way as it changes too often and cli's tests rely on it (making it part of the build/test pipeline again temporarily would be easiest I guess)

@holic
Copy link
Member Author

holic commented Apr 21, 2023 via email

@holic holic merged commit e614e37 into main Apr 24, 2023
@holic holic deleted the holic/build-scripts branch April 24, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Clean up build scripts
2 participants