Skip to content

fix(compiler-cli): allow relative rootDir#41359

Closed
literalpie wants to merge 2 commits intoangular:masterfrom
literalpie:relative-root-dir
Closed

fix(compiler-cli): allow relative rootDir#41359
literalpie wants to merge 2 commits intoangular:masterfrom
literalpie:relative-root-dir

Conversation

@literalpie
Copy link
Copy Markdown
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Issue Number: #36290

What is the new behavior?

If a relative rootDir is passed in, an error is no longer thrown

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Building off of #36291 by @Toxicable with test added and other recommended changes.

@google-cla google-cla bot added the cla: yes label Mar 28, 2021
@pullapprove pullapprove bot requested a review from AndrewKushnir March 28, 2021 17:55
@literalpie literalpie force-pushed the relative-root-dir branch 4 times, most recently from 4bf7f49 to 851ea1e Compare March 28, 2021 19:17
@AndrewKushnir AndrewKushnir requested review from JoostK and petebacondarwin and removed request for AndrewKushnir March 29, 2021 05:04
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Great! Thanks @literalpie - I think this unit test is enough.
Can you modify the commit message? It doesn't need to mention the previous PR particularly, IMO. Perhaps:

fix(compiler-cli): resolve `rootDirs` to absolute paths

Ensure that `rootDirs` are absolute by resolving them against the
current working directory.

Fixes #36290

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler target: patch This PR is targeted for the next patch release labels Mar 30, 2021
@ngbot ngbot bot modified the milestone: Backlog Mar 30, 2021
@literalpie
Copy link
Copy Markdown
Contributor Author

@petebacondarwin done!

@jbedard
Copy link
Copy Markdown
Contributor

jbedard commented Mar 30, 2021

Thanks for picking this up @literalpie!

Ensure that `rootDirs` are absolute by resolving them against the current working directory.

Fixes angular#36290
@JoostK
Copy link
Copy Markdown
Member

JoostK commented Apr 10, 2021

I once prepped an example of how this could be testing in an "integration-like" test in JoostK@c08c753, as without such a test this is easy to regress in. @literalpie could you have a look at adding a test like that?

@JoostK JoostK added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 11, 2021
@JoostK
Copy link
Copy Markdown
Member

JoostK commented Apr 11, 2021

@literalpie CI is not yet happy with the second commit; it wants it to have a short body.

this will make it easier to detect regressions of the relative rootDir behavior
@literalpie
Copy link
Copy Markdown
Contributor Author

@JoostK i think it's better now. Thanks for your help and for your patience

@JoostK JoostK added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 11, 2021
@AndrewKushnir
Copy link
Copy Markdown
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Apr 12, 2021
@zarend zarend added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Apr 12, 2021
@zarend zarend closed this in 3e0fda9 Apr 12, 2021
zarend pushed a commit that referenced this pull request Apr 12, 2021
this will make it easier to detect regressions of the relative rootDir behavior

PR Close #41359
@literalpie literalpie deleted the relative-root-dir branch April 12, 2021 17:38
@JoostK
Copy link
Copy Markdown
Member

JoostK commented Apr 12, 2021

@literalpie Thanks again for your contribution!!

@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants