Skip to content

fix(@angular/cli): command runner is not working on windows #11929

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

Merged
merged 1 commit into from
Aug 17, 2018
Merged

fix(@angular/cli): command runner is not working on windows #11929

merged 1 commit into from
Aug 17, 2018

Conversation

alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented Aug 17, 2018

fs is unable to read the normalized paths from @angular-devkit/core on windows as the disk drive is replaced from c:/ to /c/

Closes #11928

Another alternative is to use getSystemPath in all the places where the is readFileSync within the runner. But I found the current implementation to be cleaner.

This was not caught in CI, as in AppVeyor we are not running test-cli-e2e

`fs` is unable to read the normalized paths from `@angular-devkit/core`  on `windows` as the disk drive is replaced from `c:/` to `/c/`

Closes #11928
@@ -111,7 +108,7 @@ export async function runCommand(

return 1;
}
const cliDir = dirname(normalize(commandMapPath));
const cliDir = dirname(commandMapPath);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: dirname will internally normalize the path before returning.

@alexeagle
Copy link
Contributor

FYI @hansl looks like it was introduced in your 2ce1155#diff-53bc4547164f6bfb4fee01f58a298892

@alan-agius4
Copy link
Collaborator Author

On another note, is it really required to remove the : when normalizing paths?

@hansl hansl added the target: major This PR is targeted for the next major release label Aug 17, 2018
@hansl
Copy link
Contributor

hansl commented Aug 17, 2018

@alan-agius4 there's a misunderstanding here;

  1. I did not catch the change in time in my PR, sorry about that. The original PR was the one introducing the error, but I should have caught it.
  2. normalize and Path are devkit concepts. They should never be mixed with Node fs methods. Core does not understand Node fs, so it's okay. All those Path types are useable in devkit's hosts and general methods. They're meant to be platform independent, so no : or http: or anything weird like that. They just represent paths; absolute or relative, directory names separated by /. That's all. They should never be used outside of core methods.
  3. I'd like to move the Path type at some point to not be a subtype of string, but be a unique symbol. That way people won't make that mistake again. But in the meantime we have to deal with this.

@hansl hansl merged commit b759ebd into angular:master Aug 17, 2018
@alan-agius4
Copy link
Collaborator Author

@hansl thanks for the clarifications. 👍

@alan-agius4 alan-agius4 deleted the fix_runner_windows branch August 17, 2018 18:43
@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 Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ng <command> 0.8.0-beta.3 is broken on windows
4 participants