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

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

Closed
wants to merge 2 commits into from

Conversation

literalpie
Copy link
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
@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 petebacondarwin and JoostK and removed request for AndrewKushnir March 29, 2021 05:04
Copy link
Member

@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
Contributor Author

@petebacondarwin done!

@jbedard
Copy link
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
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?

packages/compiler-cli/test/ngtsc/env.ts Outdated Show resolved Hide resolved
@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
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
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
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
Member

JoostK commented Apr 12, 2021

@literalpie Thanks again for your contribution!!

@angular-automatic-lock-bot
Copy link

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.

None yet

6 participants