Skip to content

fix(compiler-cli): ensure file_system handles mixed Windows drives#37959

Closed
petebacondarwin wants to merge 5 commits into
angular:masterfrom
petebacondarwin:ngcc-windows-drive-paths
Closed

fix(compiler-cli): ensure file_system handles mixed Windows drives#37959
petebacondarwin wants to merge 5 commits into
angular:masterfrom
petebacondarwin:ngcc-windows-drive-paths

Conversation

@petebacondarwin

Copy link
Copy Markdown
Contributor

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

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

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`

Comment thread packages/compiler-cli/src/ngtsc/file_system/src/helpers.ts Outdated
Comment thread packages/compiler-cli/src/ngtsc/file_system/src/node_js_file_system.ts Outdated
Comment thread packages/compiler-cli/src/ngtsc/file_system/src/types.ts Outdated
Comment thread packages/compiler-cli/src/ngtsc/imports/src/emitter.ts Outdated
Comment thread packages/compiler-cli/src/ngtsc/file_system/test/node_js_file_system_spec.ts Outdated
@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 force-pushed the ngcc-windows-drive-paths branch from 7be6df8 to b415f89 Compare July 8, 2020 12:06
Comment thread packages/compiler-cli/src/ngtsc/file_system/src/helpers.ts Outdated
@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

alxhub commented Jul 10, 2020

Copy link
Copy Markdown
Member

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 petebacondarwin deleted the ngcc-windows-drive-paths branch July 15, 2020 15:31
@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 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