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

feat: move forge build + abi + abi-ts to out #1483

Merged
merged 12 commits into from
Sep 14, 2023
Merged

Conversation

holic
Copy link
Member

@holic holic commented Sep 14, 2023

forge has a default out directory for build artifacts. When you forge build or forge test, everything gets built and written to this directory, including tests and scripts.

For the purposes of a clean ABI directory with matching TS types, I thought I would try to side step this by separately writing forge build output to an abi dir (via --out abi) that would also skip script (.s.sol) and test (.t.sol) files (via --skip script test).

This ended up working against us in our current CLI approach, where deploys need the systems ABI artifacts to generate an IWorld interface, and uses the configured forge output dir (via forge config) to get it. So we either had to build to both places (out and abi) or build to just abi but then be missing systems ABIs because they may not have been built to out, where forge is configured. And we can't update the foundry.toml to out = abi because that would cause forge test to put its artifacts there too.

We could work around this by updating our deploy pipeline to be configurable with a forge output dir (and fall back to forge config -> out), but then we'd still have forge build --out abi --extra-files abi ... commands littered around and that felt gross too.

So I've gone the simpler route for now and just default everything to forge build (with skips for tests/scripts when we're just deploying or building for ABI artifacts) and make sure to include the proper ABI files we need in our npm package artifacts.

This also moves src/MudTest.sol to test/MudTest.t.sol so it's included in --skip test and we don't have to manually skip it. Since this isn't used in store package and has a reference to a world, should we move this to the world package instead?

@changeset-bot
Copy link

changeset-bot bot commented Sep 14, 2023

🦋 Changeset detected

Latest commit: bd04598

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 28 packages
Name Type
@latticexyz/store Major
@latticexyz/world Major
create-mud Major
@latticexyz/cli Major
@latticexyz/dev-tools Major
@latticexyz/react Major
@latticexyz/store-indexer Major
@latticexyz/store-sync Major
@latticexyz/abi-ts Major
@latticexyz/block-logs-stream Major
@latticexyz/common Major
@latticexyz/config Major
@latticexyz/ecs-browser Major
@latticexyz/gas-report Major
@latticexyz/network Major
@latticexyz/noise Major
@latticexyz/phaserx Major
@latticexyz/protocol-parser Major
@latticexyz/recs Major
@latticexyz/schema-type Major
@latticexyz/services Major
@latticexyz/solecs Major
solhint-config-mud Major
solhint-plugin-mud Major
@latticexyz/std-client Major
@latticexyz/std-contracts Major
@latticexyz/store-cache Major
@latticexyz/utils Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@holic holic changed the title fix(cli): build to abi when deploying feat: move forge build + abi + abi-ts to out Sep 14, 2023
@alvrs
Copy link
Member

alvrs commented Sep 14, 2023

This also moves src/MudTest.sol to test/MudTest.t.sol so it's included in --skip test and we don't have to manually skip it. Since this isn't used in store package and has a reference to a world, should we move this to the world package instead?

good point, should be in world!

alvrs
alvrs previously approved these changes Sep 14, 2023
Comment on lines -2 to -3
**/abi/**/*.json linguist-generated=true
**/abi/**/*.json.d.ts linguist-generated=true
Copy link
Member

Choose a reason for hiding this comment

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

should we suppress out diffs now then?

Copy link
Member

Choose a reason for hiding this comment

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

ahh i see abis are no longer checked in to git

@@ -5,11 +5,11 @@
"license": "MIT",
"scripts": {
"build": "pnpm run build:mud && pnpm run build:abi && pnpm run build:abi-ts",
"build:abi": "forge build --extra-output-files abi --out abi --skip test script MudTest.sol",
"build:abi-ts": "mud abi-ts --input 'abi/IWorld.sol/IWorld.abi.json' && prettier --write '**/*.abi.json.d.ts'",
"build:abi": "forge build --skip test script",
Copy link
Member

Choose a reason for hiding this comment

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

do we not need the --extra-output-files abi anymore?

Copy link
Member

Choose a reason for hiding this comment

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

ahh i see it's in the forge config now

@holic holic marked this pull request as ready for review September 14, 2023 17:50
@holic holic requested a review from dk1a as a code owner September 14, 2023 17:50
@@ -2,10 +2,10 @@
pragma solidity >=0.8.0;
Copy link
Member

Choose a reason for hiding this comment

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

just realized this and other tests here are still using 0.8.0, should we update to 0.8.21?

Copy link
Member Author

@holic holic Sep 14, 2023

Choose a reason for hiding this comment

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

we haven't merged the solc stuff yet (pending changeset and some more testing)

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