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: fix turbo clean #1047

Closed
wants to merge 4 commits into from
Closed

build: fix turbo clean #1047

wants to merge 4 commits into from

Conversation

holic
Copy link
Member

@holic holic commented Jun 17, 2023

reverts #1045 #1046 in favor of turbo clean

not sure how to repro the original issue to verify this approach works

if this doesn't work, another possible option is to have the mud codegen step (build:mud) do a rimraf src/codegen before running, so its part of the turbo build cache

@changeset-bot
Copy link

changeset-bot bot commented Jun 17, 2023

⚠️ No Changeset found

Latest commit: 80c15be

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@holic holic marked this pull request as ready for review June 17, 2023 01:22
@holic holic requested a review from alvrs as a code owner June 17, 2023 01:22
@alvrs
Copy link
Member

alvrs commented Jun 17, 2023

Conceptually, if we clear the artifacts in the setup step, wouldn't we always have to rebuild, so we could never benefit from the turbo cache in CI? If so, I feel like it would be sufficient to do a full clean+rebuild in one action (eg. "Build and validate artifacts") and use the cache in all other actions

@holic
Copy link
Member Author

holic commented Jun 18, 2023

Conceptually, if we clear the artifacts in the setup step, wouldn't we always have to rebuild, so we could never benefit from the turbo cache in CI? If so, I feel like it would be sufficient to do a full clean+rebuild in one action (eg. "Build and validate artifacts") and use the cache in all other actions

My understanding when running this locally is that the build is still cached. I can run pnpm clean and pnpm build and the 2nd or 3rd time the build is run, it'll be "full turbo". I think the difference is it starts the build from a clean output, so the cached output should be a little more accurate/what is intended? I can't say for sure without a repro case.

I can also see it being useful to have an uncached build in CI to ensure we get fully accurate results, and it should probably be tied to the same job that does releases so we can ensure a clean and complete set of artifacts for npm, etc.?

(I don't feel strongly about any of this, just thought I'd take a stab at trying to teach turbo a bit more about our intentions to see what the results were.)

@holic
Copy link
Member Author

holic commented Jun 19, 2023

Another thing to note about the composite steps is that the get combined into a single entry in the CI logs, so it's a bit hard to figure out how much time is spent on the "sub-steps" of each composite action.

So anything using the "Build" composite action will only see a "Build" step in CI rather than "Cache turbo build", "Build", "Outdated files ..." (the last one is the main one I was hoping to easily surface as a failure reason for the step).

@holic
Copy link
Member Author

holic commented Jun 20, 2023

after #1057 is merged, I could potentially just use this PR as a revert for adding the "clean" steps in CI, since we shouldn't really need em anymore (pretty much all builds are either gitignored or clean themselves)

@holic holic marked this pull request as draft June 20, 2023 14:03
@holic holic closed this Jun 26, 2023
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