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: turborepo #669

Merged
merged 26 commits into from
Apr 25, 2023
Merged

build: turborepo #669

merged 26 commits into from
Apr 25, 2023

Conversation

holic
Copy link
Member

@holic holic commented Apr 24, 2023

continues #661

vercel deploy fix in #674, will bring that in once merged

@vercel
Copy link

vercel bot commented Apr 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mud ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 25, 2023 7:58pm

refactor: change docs package.json private to false

fix: build:docs

fix: testing suite required jest-environment-jsdom with newer versions
of jest

refactor: run tests through turbo

chore: turbo other tasks
chore: remove quotes around "private"

chore: run sort-package-json

chore: seems like vercel might need this path

chore: clean up changes, make install scripts hidden

chore: remove extra code from turbo file

chore: sleepy rebase
path: .turbo
key: ${{ runner.os }}-turbo-${{ github.sha }}
restore-keys: |
${{ runner.os }}-turbo-
Copy link
Member Author

Choose a reason for hiding this comment

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

pulled this from vercel/turborepo#2761

but runner.os being the only unique key for the cache seems a little aggressive? is turbo smart enough to figure out what cached things to use across all our builds/commits?

Copy link
Contributor

@Handfish Handfish Apr 25, 2023

Choose a reason for hiding this comment

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

I forked austinwoon's demo, took a look at his commits and recreated his build examples so I could view the logs. I think turbo is smart enough to cache properly with the runner.os restore-key.

Turbo acts as if it would on your local machine. It performs a check to see if the cache is relevant. If changes are made it will build that specific package rather than using the cache.

Copy link
Contributor

@Handfish Handfish Apr 25, 2023

Choose a reason for hiding this comment

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

Branch name might be useful though because in the case two people are committing at the same time they will be fighting over the cache. According to the docs, multiple restore-keys can be provided and act as fallback keys on miss, so maybe it'd be good to take advantage of that.

@holic holic marked this pull request as ready for review April 25, 2023 00:37
@holic holic requested a review from a user April 25, 2023 00:37
@holic holic requested a review from alvrs as a code owner April 25, 2023 00:37
@holic holic requested review from roninjin10 and dk1a April 25, 2023 00:37
@holic
Copy link
Member Author

holic commented Apr 25, 2023

cc @Handfish

"commit": "cz",
"dev": "turbo run dev --concurrency 20",
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious how this works / is supposed to work. It seems like it runs dev independently for each package, so if I for example change things in world, only world rebuilds, but I would have expected all downstream dependencies (eg cli) to also rebuild? or is this not the intention of this command?

Copy link
Member

Choose a reason for hiding this comment

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

Related to the comment above (but not strictly related to this PR) - should we add forge build --watch to the dev commands of our contract packages to rebuild those too (so in the ideal case a rebuild of downstream dependencies, eg CLI or network, would be triggered)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like it runs dev independently for each package, so if I for example change things in world, only world rebuilds, but I would have expected all downstream dependencies (eg cli) to also rebuild? or is this not the intention of this command?

I'm not sure (first time using turbo/nx) but this is what I'd expect too, and something we can iterate on/improve as we go!

should we add forge build --watch to the dev commands of our contract packages to rebuild those too (so in the ideal case a rebuild of downstream dependencies, eg CLI or network, would be triggered)

Probably! But there are other things ahead of forge build that don't have watch commands yet like tablegen, worldgen, etc. so I was kinda waiting on those first. But again, we can improve this as we go.

Copy link
Member

Choose a reason for hiding this comment

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

But there are other things ahead of forge build that don't have watch commands yet like tablegen, worldgen, etc. so I was kinda waiting on those first.

Agree those need --watch too, but forge build --watch could be useful by itself for all solidity changes that don't impact the interface and mud config

Copy link
Member

@alvrs alvrs left a comment

Choose a reason for hiding this comment

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

had one question about dev, but this is a great improvement for already even just for build, so happy to merge as is and look at dev in a follow up

@holic
Copy link
Member Author

holic commented Apr 25, 2023

FYI, looks like Vercel attempts to detect turbo.json and uses turbo run build rather than pnpm run build as its default entry point, which doesn't work since I've moved docs out of the workspace. So I had to configure the mud docs vercel app to use pnpm run build as its build command.

image

I'll follow up soon to see if docs can safely move back into the workspace.

@holic holic merged commit 09b80e6 into main Apr 25, 2023
@holic holic mentioned this pull request Apr 25, 2023
4 tasks
@roninjin10
Copy link
Contributor

Very nice!

# https://github.com/foundry-rs/foundry/issues/4736
- name: Install solc
shell: bash
run: cargo install svm-rs && svm install 0.8.13
Copy link
Member Author

Choose a reason for hiding this comment

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

@holic holic deleted the holic/turborepo branch June 23, 2023 11:34
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.

4 participants