[rush] Pass subspace parameter directly to SelectionParameterSet #4700
Conversation
| public async getSelectedProjectsAsync(terminal: ITerminal): Promise<Set<RushConfigurationProject>> { | ||
| public async getSelectedProjectsAsync( | ||
| terminal: ITerminal, | ||
| subspaceName?: string |
There was a problem hiding this comment.
Instead of passing subspaceName, it would be more consistent for the constructor of SelectionParameterSet to accept an optional flag that tells it to create the special --subspace parameter. The design seems to be that SelectionParameterSet creates and owns its parameters rather than receiving their values as inputs to getSelectedProjectsAsync(), and it seems to me that --subspace is really just another selection parameter (albeit degenerate because it is only supported by some commands and uses a different syntax).
There was a problem hiding this comment.
The main reason that SelectionParameterSet owns its own parameters is for DRY, since otherwise we were writing the same selection values over and over again.
There was a problem hiding this comment.
As of PR #4701, getSelectedProjectsAsync() calculates expressions using PNPM CLI operators like --filter ...my-app. These selections will malfunction with subspaces. As a workaround, when subspaces are enabled the args get overwritten with a flat list. For details, see fb2c242
In a future PR, Rush will need to (1) calculate the flat list of projects (without relying on PNPM operators) and (2) somehow communicate this list to pnpm install without exceeding the Windows maximum command length.
| terminal, | ||
| parameterName: 'subspace' | ||
| }) | ||
| : []; |
There was a problem hiding this comment.
@dmichon-msft is there a perf concern if this sort of expression explodes into a set with 1000+ items? Does the Selection system have some efficient way to handle such sets? Or is it fast in practice?
There was a problem hiding this comment.
If it becomes a problem we can replace the project selection with a bitvector, which makes set theoretic operations much faster.
Selection.union is O(elements)
Selection.expandAllDependencies is O(projects * directDependencies)
Selection.expandAllConsumers is O(projects * directConsumers)
So performance is bounded at O(projects^2). We can profile, but likely not a significant issue compared to some of the places in Webpack that were doing complex set arithmetic.
| public async getSelectedProjectsAsync(terminal: ITerminal): Promise<Set<RushConfigurationProject>> { | ||
| public async getSelectedProjectsAsync( | ||
| terminal: ITerminal, | ||
| subspaceName?: string |
There was a problem hiding this comment.
The main reason that SelectionParameterSet owns its own parameters is for DRY, since otherwise we were writing the same selection values over and over again.
| terminal, | ||
| parameterName: 'subspace' | ||
| }) | ||
| : []; |
There was a problem hiding this comment.
If it becomes a problem we can replace the project selection with a bitvector, which makes set theoretic operations much faster.
Selection.union is O(elements)
Selection.expandAllDependencies is O(projects * directDependencies)
Selection.expandAllConsumers is O(projects * directConsumers)
So performance is bounded at O(projects^2). We can profile, but likely not a significant issue compared to some of the places in Webpack that were doing complex set arithmetic.
|
This PR looks like a better approach than #4689. We should close that PR and pursue this one. |
william2958
force-pushed
the
will/subspace-patch-24
branch
from
f184d7a
to
397ee30
Compare
william2958
requested review from
iclanton,
apostolisms and
D4N14L
as code owners
common/changes/@microsoft/rush/will-subspace-patch-24_2024-05-13-20-35.json
Outdated
Show resolved
Hide resolved
…13-20-35.json Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
| console.log(); | ||
| // eslint-disable-next-line no-console | ||
| console.log( | ||
| Colorize.red( | ||
| `The "${this._subspaceParameter?.longName}" parameter can only be passed if "subspacesEnabled" is set to true in subspaces.json.` | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Sorta weird that stuff may be printed to the console when someone calls a get<something> function.
There was a problem hiding this comment.
🤔 True.
But this is a private API only used by CLI parsing code. And it's conceptually throwing an exception, just using the AlreadyReportedError formalism for better formatting.
We could rename getSelectedProjectsAsync() to something like getAndValidateProjectsAsync() but I wouldn't bother in this PR.
| // Enable filtering to reduce evaluation cost | ||
| enableFiltering: true | ||
| }, | ||
| false |
There was a problem hiding this comment.
Why can't someone run rush build --subspace my-subspace?
There was a problem hiding this comment.
They can do rush build --only subspace:my-subspace. But that is an "unsafe" selector, because it does not guarantee that dependencies get built. Thus a convenient --subspace macro isn't a good idea.
…ubspace:x" and change preventSelectingAllSubspaces to consider the command-line rather than the final selection
Make "rush install --subspace x" equivalent to "rush install --only subspace:x"
octogonz
changed the title
Summary
This MR addresses an issue where currently when the "--subspace" parameter is passed to the install or update action, i.e. rush install --subspace my_subspace, all projects in the entire monorepo will be selected to be installed.
Details
This issue was resolved by considering the "--subspace" cli parameter from BaseInstallAction as a selection, so the rush engine can detect that there are filters applied and doesn't try to do a global install. This MR also changes the way the "--subspace" parameter is treated, instead of a unique selector, it basically just maps to the same action as using the "--only subspace:my-subspace" selector.
How it was tested
This was tested on an existing subspace monorepo that was experiencing this issue.
Impacted documentation