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: Support customizing CI builds from Dev builds #2283

Open
1 task
ifeanyiecheruo opened this issue Oct 14, 2020 · 4 comments
Open
1 task

rush: Support customizing CI builds from Dev builds #2283

ifeanyiecheruo opened this issue Oct 14, 2020 · 4 comments
Labels
enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem

Comments

@ifeanyiecheruo
Copy link

  • [ x ] Feature
  • Bug

I need support for the ability to declare what should happen in a project when rush build and rush rebuild are executed vs when rushx build or npm script build are executed

Scenarios

  • For day to day dev use and for IDE integration: I would want

    • rushx build to clean and build a specific project
    • rushx test run tests for a built project
    • rushx start to optionally run the project
      I want this so that my devs can quickly edit and prototype code and pay the cost of linting and testing when they choose to.
  • For day to day pre-push validation: I would want

  • rush build to clean, build, lint, and test only changed projects
    Which is the behavior we have today.
    I want this so that my devs can reasonably predict what is going to happen in CI without rebuilding the entire mono-repo

  • For CI: I would want

  • rush rebuild to clean, build, lint, and test all projects
    Which is the behavior we have today

The above scenarios are in conflict today.
If I edit build in package.json for the dev scenario, I will break the pre-push and CI senario.

Since rushx is the wrapper for running package.json scripts we don't want to change its behavior.
So it seem reasonable that we give rush the ability to optionally declare what should happen to a project it is building or rebuilding

Hopefully my scenarios make sense, let me know if this is a reasonable thing to support

@octogonz octogonz added needs design The next step is for someone to propose the details of an approach for solving the problem enhancement The issue is asking for a new feature or design change labels Oct 16, 2020
@octogonz
Copy link
Collaborator

octogonz commented Oct 19, 2020

The relationship between rush build and rushx build has always been a little weird. The commands that make sense for one project are somewhat different from what makes sense for the bulk commands for many projects. For example, we have rush rebuild but no rushx rebuild. Prior to Rush 4, rush build would actually invoke the "test" script and fallback to "build" if it was missing.

However -- considering the design proposal for multiphase builds -- maybe things are changing. One of the goals was to make rush build less magical. Instead, it could be modeled just like a custom command. We discussed eliminating rush rebuild and replacing it with a rush clean or rush rebuild --clean, so that incremental custom commands can get cleaned as well.

So maybe rush build should eventually change its meaning to "run rushx build for every project".

And CI jobs would instead run rush test, which will mean "run rushx test for every project".

@ifeanyiecheruo I think you are right to expect npm run build and npm run test to behave consistently with other Node.js projects. Since we'd need a MAJOR version bump to change the rush build semantics, something like this seems like a reasonable short-term solution:

"script": {
  "build": "heft build",
  "test": "heft test",
  "start": "heft start",
  "rush-build": "heft test --clean",  // <-- what "rush build" does
}

However PR #2298 takes this a step further, essentially allowing a custom command to specify an arbitrary array of package.json script names to try. That seems maybe too flexible. It would allow a mapping like this:

"script": {
  "build": "heft build",  // <-- what "rush test" does 🤔
  "test": "heft test",    // <-- what "rush build" does 🤔
  "start": "heft start"
}

It makes sense to generalize the rush-build mapping to work with any custom bulk command. But instead of an arbitrary array, what if we simply said that bulk commands look for "rush-<actionName>" first, and then fall back to "<actionName>". Imposing this constraint might make the meaning of rush foo more understandable to users. They don't need to look in custom-commands.json to guess that it's going to run "rush-foo" or "foo".

@iclanton
Copy link
Member

This is an interesting problem. I actually started work on a feature we've talked about for a while we've been calling "multi-phase builds," which provides more flexibility in what custom commands do when executed. I'm still working on the updates to Rush's scheduler, but here's a PR that includes the updates to the command-line.json schema: #2299

I really like the idea of aligning rushx <verb>'s behavior with rush <verb>'s behavior (if I'm understanding the problem correctly). This would be a breaking change to what rushx does, but I could see a repo owner defining a test command in command-line.json that runs two phases: build (which depends on upstream projects' build phases) and test (which depends on its own project's build phase). Running rush test would run projects' build scripts the way Rush does today, and then run projects' test scripts in parallel to downstream projects' build commands. Extending this to rushx, one would expect that running rushx test would run the phases defined for test in command-line.json, but only for a single project. So that would mean running the build script and then the test script for that project.

@hbo-iecheruo
Copy link

hbo-iecheruo commented Oct 19, 2020

I'm not sure multi-phased builds resolves with the problem I'm trying to solve

rush build <- does all the work needed across all projects for CI (clean, build, test)
rushx build <- is forced to do what rush build does but scoped to a single project (clean, build, test) which is not what my devs want

My proposal is to provide a script override for rush so that rush build and rushx build can do different things.

To the best of my knowledge even with phased builds rush build would be able to assemble and efficiently schedule the dependencies of a test phase (clean and build) but rushx build would still be forced to do the thing my devs don't want to do - run tests.

One possible solution is to always be explicit about what scripts a rush command like build executes ["clean", "build", "test"] and allow rush to compose them over the set of projects in the mono repo, while allowing rushx to execute those build atoms directly

@octogonz
Copy link
Collaborator

octogonz commented Nov 2, 2020

To the best of my knowledge even with phased builds rush build would be able to assemble and efficiently schedule the dependencies of a test phase (clean and build) but rushx build would still be forced to do the thing my devs don't want to do - run tests.

@hbo-iecheruo's concern is valid. We discussed it in the latest meeting about multiphase commands, and I believe it's been solved. The full details were written up as #2300, but the gist is that:

  • rushx build and rush build will NOT run tests
  • rushx test and rush test WILL run tests
  • The underlying shell commands (e.g. for rushx build versus rush build) will be specified separately. Thus rushx build can support lots of extra parameters that do not make sense for the multi-project rush build command line.

@microsoft microsoft deleted a comment from gfortaine Jan 8, 2021
@iclanton iclanton moved this to General Discussions in Bug Triage Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem
Projects
Status: General Discussions
Development

No branches or pull requests

4 participants