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

[rush] Revise command line selection operations #2422

Merged
merged 26 commits into from
Feb 1, 2021

Conversation

dmichon-msft
Copy link
Contributor

@dmichon-msft dmichon-msft commented Jan 8, 2021

Summary

Adds support for --from-except and --to-except to invoke a command on all dependents on a specific project, or all dependencies of a specified project.

Adds support for --only PROJECT to invoke a command on PROJECT without adding its dependents or dependencies to the selection.

Adds support for --affected-by PROJECT and --affected-by-except PROJECT to provide the functionality formerly provided by --from (i.e. execute the command on PROJECT and all dependents thereof, without reprocessing the dependencies).

Updates the interaction between --from and --to in TaskSelector to intersect the results of the former with the latter.
This ensures that rush build --from A --to B builds all projects that depend on A (including A) and are dependencies of B (including B). This is intended to support watch scenarios where some files have changed in A and the developer is interested in the effects on B.

Adds new properties to RushConfigurationProject that contain the dependencies and dependents as ReadonlySet<RushConfigurationProject> for ease of graph analysis.
Factors out directed graph manipulation functions for compositing. Adds unit tests for the same.
Switches to using generators for collection manipulation where appropriate.
Adds --with-dependencies flag to include all dependencies of the selected projects.
Fixes #2354
Alters behavior of --from to include all dependencies per #2349

Details

Name of parameters subject to revision, pending discussion in #2354.
Optimized the calculation of the dependency graph to use a loop instead of recursion.

Moves responsibility for evaluating --from, --to, etc. out of TaskSelector and into the calling command, with the aid of selection helper functions. This change is to allow --watch to specify exactly the set of projects it wishes to execute.

How it was tested

Invoked a variety of rush build scenarios with a combination of --from, --to, --to-except, --affected-by, --affected-by-except, --only on the @rushstack repo.
Selection helpers have unit test coverage.

Edit: fixed issue reference
Edit 2: reverted behavioral changes to --to and --from
Edit 3: added --with-dependencies flag
Edit 4: removed --with-dependencies flag and made it the default behavior of --from. Removed --from-except since including dependencies makes the option nonsense.

@iclanton
Copy link
Member

@dmichon-msft #1234 doesn't seem like the right reference.

@dmichon-msft
Copy link
Contributor Author

@dmichon-msft #1234 doesn't seem like the right reference.

Fixed reference to 2354

@octogonz
Copy link
Collaborator

Thanks a ton for making this PR! I really need this feature. :-)

Copy link
Collaborator

@octogonz octogonz left a comment

Choose a reason for hiding this comment

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

Based on the PR discussion, this PR is making a nontrivial design change that conflicts with #2349

We need to sort that out before merging.

@dmichon-msft dmichon-msft changed the title [@microsoft/rush] Rework --to, --from, add --to-except, --from-except [@microsoft/rush] Add --to-except, --from-except Jan 21, 2021
@wbern
Copy link
Contributor

wbern commented Jan 25, 2021

Thanks for the work done here! Running into these issues myself now with --from flag.

Any chance this could be done for the install command as well?

@dmichon-msft
Copy link
Contributor Author

@octogonz , give this another look with the reverted behavior and new version of the internals?

@wbern
Copy link
Contributor

wbern commented Jan 27, 2021

With regards to my comment in #2345

@octogonz Honestly I think --from should include everything that's needed. As a colleague of mine said, to and from makes an hourglass, or at least so we thought at first. 🙂

Would it be drastic to let --from be "--with-dependencies", and have a flag that says "--without-dependencies"? I want to merge this really badly, so it pains me to bring this up, but a good design is important too. 😄

@octogonz
Copy link
Collaborator

Would it be drastic to let --from be "--with-dependencies", and have a flag that says "--without-dependencies"? I want to merge this really badly, so it pains me to bring this up, but a good design is important too. 😄

Yes, I was actually thinking the same thing. Do the safe internally consistent thing by default, and then the extra CLI parameter would be for people know what they are doing. We could even argue that it's not exactly a breaking design change, to broaden the set of projects. (PR #2349 had assumed that was the intended behavior, for example.)

We can think a little more about whether --without-dependencies is the best name. But with this plan, PR #2435 seems good to go.

@dmichon-msft
Copy link
Contributor Author

@wbern , with latest iteration, --from makes a truncated cone, and --to X --from X is redundant, since it does the same thing as --from X.

Adding a --without-dependencies or such can be left as a later exercise, since I solved my issue for --watch by having TaskSelector accept the exact set of projects to execute the task on.

@octogonz , latest iteration should now resolve the behavior conflict.

@octogonz
Copy link
Collaborator

Fixes #1447

@iclanton
Copy link
Member

All of the generating functions feel more complicated than necessary.

@octogonz
Copy link
Collaborator

Here's an initial draft of the docs for @dmichon-msft's feature.

@dmichon-msft
Copy link
Contributor Author

Here's an initial draft of the docs for @dmichon-msft's feature.

@octogonz , is there supposed to be a link here?

@octogonz
Copy link
Collaborator

octogonz commented Feb 1, 2021

@dmichon-msft looks like I forgot to paste it: :-)

https://rushjs.io/pages/developer/selecting_subsets/

@wbern
Copy link
Contributor

wbern commented Feb 1, 2021

@dmichon-msft looks like I forgot to paste it: :-)

https://rushjs.io/pages/developer/selecting_subsets/

Looks really good!

@dmichon-msft
Copy link
Contributor Author

@octogonz , I believe I have addressed all of your comments at this point.

@octogonz octogonz merged commit 52d680e into microsoft:master Feb 1, 2021
@octogonz
Copy link
Collaborator

octogonz commented Feb 1, 2021

Released with Rush 5.38.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[rush] Add a "rush build" CLI parameter for building a project's dependencies but NOT the project
5 participants