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] rush build --from does not build transitive dependencies #1447

Closed
mikeharder opened this issue Aug 5, 2019 · 10 comments
Closed

[rush] rush build --from does not build transitive dependencies #1447

mikeharder opened this issue Aug 5, 2019 · 10 comments
Labels
bug Something isn't working as intended effort: easy Probably a quick fix. Want to contribute? :-) help wanted If you're looking to contribute, this issue is a good place to start!

Comments

@mikeharder
Copy link
Contributor

mikeharder commented Aug 5, 2019

Overview

Consider a dependency graph where B depends on A, and D depends on both B and C:

A <- B <- D
     C <- D

Say I am making changes to B, and want to verify that B and all its downstream dependencies can build correctly. I use the following command:

rush build --to B --from B

Expected

Rush should build all projects downstream of B, including their transitive dependencies (which are required for them to build). D is downstream of B so it must be built, and C is upstream of D so it must also be built.

Actual

Rush does not build C.

Workarounds

Without building the transitive dependencies of downstream projects, the only workaround I see is to build all projects in the monorepo.

Repo

https://github.com/mikeharder/rush-to-from

@iclanton
Copy link
Member

Looking into this. So the dependency graph for your repro repo is:

D --> C (not built)
  `--> B --> A

This illustrates a weirdness with the --from parameter, where Rush doesn't ensure the dependencies of projects it's building are built. I think we have two options for addressing this:

  • Change the behavior of the --from parameter to also include all other dependencies of projects that are to be built. This will be a breaking change.
  • Add the aforementioned behavior with an additional flag (something like --ensure-dependencies-are-built, but with a better name) to avoid making a breaking change.

I'm not sure which approach I prefer, or if there are any additional options.

@octogonz, @apostolisms - thoughts?

@mikeharder mikeharder changed the title [rush] rush build --to does not build transitive dependencies [rush] rush build --from does not build transitive dependencies Aug 21, 2019
@mikeharder
Copy link
Contributor Author

@iclanton: Thanks for taking a look. I just noticed the title of this issue incorrectly said --to instead of --from (now corrected), but you are correct the issue is with --from not ensuring dependencies are built.

We would be fine with either option (changing --from or additional flag). If one option could be implemented more quickly (say the additional flag since it's a non-breaking change), we would prefer that option.

@octogonz
Copy link
Collaborator

@nickpape FYI

@octogonz
Copy link
Collaborator

octogonz commented Aug 23, 2019

This picture looks exactly correct for rush build --to B --from B:

D --> C (not built)
  `--> B --> A
  • --to B means "build B, plus the things that B depends on". That set is { A, B }.
  • --from B means "build B, plus all the things that depend on B" That set is { B, D }.

When you specify both parameters, you get the union { A, B, D }.

That union is meaningful and useful. For example, suppose I did a clean clone of the repo, and I made some changes to B. The --to B ensures that I build all the prerequisites for building B. The --from B ensures that I run all the unit tests that could possibly by broken by my change. In this scenario, building C is irrelevant and potentially would waste a lot of time.

@octogonz octogonz added PR: Waiting for author We reviewed your PR but had questions/suggestions. Let us know when you're ready for another look. needs more info We can't proceed because we need a better repro or an answer to a question and removed PR: Waiting for author We reviewed your PR but had questions/suggestions. Let us know when you're ready for another look. labels Aug 23, 2019
@octogonz
Copy link
Collaborator

octogonz commented Aug 23, 2019

Ohhhh... wait a sec, I get it. The problem is that D fails to build because C wasn't built. Duh. :-P

@octogonz
Copy link
Collaborator

Yeah, let's fix this.

@octogonz octogonz added bug Something isn't working as intended effort: easy Probably a quick fix. Want to contribute? :-) help wanted If you're looking to contribute, this issue is a good place to start! and removed needs more info We can't proceed because we need a better repro or an answer to a question labels Aug 23, 2019
@antl3x
Copy link

antl3x commented Dec 6, 2019

No updates?

@arikon
Copy link

arikon commented Dec 14, 2020

What is the progress?

@octogonz
Copy link
Collaborator

This will be fixed by PR #2422

@mikeharder
Copy link
Contributor Author

Closing issue since PR #2422 has been merged

@iclanton iclanton moved this to Closed 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
bug Something isn't working as intended effort: easy Probably a quick fix. Want to contribute? :-) help wanted If you're looking to contribute, this issue is a good place to start!
Projects
Archived in project
Development

No branches or pull requests

5 participants