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

Phased Builds DIFF (do not merge) #1

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Phased Builds DIFF (do not merge) #1

wants to merge 24 commits into from

Conversation

elliot-nelson
Copy link
Owner

Demo

Example PR showing the changes made to enable phased builds.

The main branch is buildable with current rush (rush install && rush build). It has some setup (for example, it adds the "_phase:x" command definitions to the package.json files), but doesn't include anything that would be invalid configuration for current rush.

This phased branch is not buildable with current rush, but in theory, it is buildable with a rushstack branch that implements the spec described in microsoft/rushstack#2300.

@@ -168,6 +168,54 @@
// }
],

/* PHASES */
"phases": [

Choose a reason for hiding this comment

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

How are these phases mapped to Rush actions? I think we're missing the definition in "commands".

Choose a reason for hiding this comment

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

If this is a realistic example, we should show the intended Rush shell commands for realistic everyday tasks:

  • How do I build everything including all phases?
  • How do I update the README and do nothing else?
  • How do I do a --lite build where we skip unit tests and linting?
  • For these things, what's the equivalent of rush rebuild?

Maybe the answer is that initially each of these problems requires a completely new command action name. (rush build-lite?) But if so we can still enumerate them to get a sense of how painful that feature cut will be in practice.

Copy link
Owner Author

Choose a reason for hiding this comment

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

These are great questions.

Since presumably the monorepo had this --lite need before phases, I implemented a solution in main (a custom build script that lets it properly handle --lite from the rushx command line).

That --lite flag is rough for the phased approach though. As an example, I imagined that the user wanted build.js to pass --lite both to Heft and to the push_notes.js custom script. If I invent a new phased command build-lite, then it can have phases ['_phase:compile', '_phase:readme_updater'] etc., and you don't need to pass --lite to Heft because you've baked that into the phase. But that's not generically possible if you were doing something with the --lite flag.

I guess one possible approach is to implement lite phases, push-notes and push-notes-lite, and only include the appropriate one. But they'll never share build cache, and would have to be leaf nodes (no dependents), otherwise how would a dependent phase know which one to rely on.

@elliot-nelson
Copy link
Owner Author

@octogonz OK, this should be ready for another pass.

Rough summary:

  • I've gone beyond the scope of the spec and added proposed properties addPhasesToCommand and skipPhasesForCommand to the parameter definition. Perhaps the details need more work, but I feel strongly that this functionality needs to be part of the initial MVP. Working around the lack of this functionality is too much of a hack to ask people onboarding to phases, I think.

  • Added your proposed structure to the rush-project.json file. This was a good suggestion because among other things, I forgot that other bulk commands (which can still use the build cache) still need a place to get their non-phased output folders from.

  • Minor tweaking of build script names, contents, and phase names to make them more consistent.

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.

3 participants