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): ensure file_system handles mixed Windows drives #37959

Closed

Conversation

petebacondarwin
Copy link
Member

The fs.relative() method assumed that the file-system is a single tree,
which is not the case in Windows, where you can have multiple drives,
e.g. C:, D: etc.

This commit changes fs.relative() so that it no longer forces the result
to be a PathSegment and then flows that refactoring through the rest of
the compiler-cli (and ngcc). The main difference is that in some cases
we needed to check whether the result is "rooted", i.e and AbsoluteFsPath,
rather than a PathSegment, before using it.

Fixes #36777

@petebacondarwin petebacondarwin added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release comp: ngcc area: compiler Issues related to `ngc`, Angular's template compiler labels Jul 7, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jul 7, 2020
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Nice and clean! (I was afraid it would get more messy 😅)
Some minor suggestions; otherwise LGTM 🚀

Also, commit message typo: and AbsoluteFsPath --> an `AbsoluteFsPath`

@gkalpak gkalpak added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 8, 2020
The `fs.relative()` method assumed that the file-system is a single tree,
which is not the case in Windows, where you can have multiple drives,
e.g. `C:`, `D:` etc.

This commit changes `fs.relative()` so that it no longer forces the result
to be a `PathSegment` and then flows that refactoring through the rest of
the compiler-cli (and ngcc).  The main difference is that now, in some cases,
we needed to check whether the result is "rooted", i.e an `AbsoluteFsPath`,
rather than a `PathSegment`, before using it.

Fixes angular#36777
@petebacondarwin petebacondarwin added action: presubmit The PR is in need of a google3 presubmit action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 8, 2020
@petebacondarwin petebacondarwin removed the request for review from alxhub July 8, 2020 16:12
@alxhub
Copy link
Member

alxhub commented Jul 10, 2020

Caretaker: this will need cl/320619026 in order to be synced successfully (manually run the sync tool from a client with this change).

@alxhub alxhub added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jul 10, 2020
@petebacondarwin petebacondarwin removed the action: presubmit The PR is in need of a google3 presubmit label Jul 13, 2020
@petebacondarwin petebacondarwin added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Jul 13, 2020
@atscott atscott closed this in 6b31155 Jul 13, 2020
petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Jul 13, 2020
…ngular#37959)

The `fs.relative()` method assumed that the file-system is a single tree,
which is not the case in Windows, where you can have multiple drives,
e.g. `C:`, `D:` etc.

This commit changes `fs.relative()` so that it no longer forces the result
to be a `PathSegment` and then flows that refactoring through the rest of
the compiler-cli (and ngcc).  The main difference is that now, in some cases,
we needed to check whether the result is "rooted", i.e an `AbsoluteFsPath`,
rather than a `PathSegment`, before using it.

Fixes angular#36777

PR Close angular#37959
@petebacondarwin petebacondarwin deleted the ngcc-windows-drive-paths branch July 15, 2020 15:31
@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 Aug 15, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…ngular#37959)

The `fs.relative()` method assumed that the file-system is a single tree,
which is not the case in Windows, where you can have multiple drives,
e.g. `C:`, `D:` etc.

This commit changes `fs.relative()` so that it no longer forces the result
to be a `PathSegment` and then flows that refactoring through the rest of
the compiler-cli (and ngcc).  The main difference is that now, in some cases,
we needed to check whether the result is "rooted", i.e an `AbsoluteFsPath`,
rather than a `PathSegment`, before using it.

Fixes angular#36777

PR Close angular#37959
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 merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal Error: relativeFrom(....../my-app/node_modules/typescript/lib/lib.d.ts): path is not relative
5 participants