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(ngcc): handle compilation diagnostics #31996

Closed
wants to merge 2 commits into from

Conversation

@JoostK
Copy link
Member

commented Aug 4, 2019

Previously, any diagnostics reported during the compilation of an
entry-point would not be shown to the user, but either be ignored or
cause a hard crash in case of a FatalDiagnosticError. This is
unfortunate, as such error instances contain information on which code
was responsible for producing the error, whereas only its error message
would not. Therefore, it was quite hard to determine where the error
originates from.

This commit introduces behavior to deal with error diagnostics in a more
graceful way. Such diagnostics will still cause the compilation to fail,
however the error message now contains formatted diagnostics.

Closes #31977
Resolves FW-1374

@ngbot ngbot bot added this to the Backlog milestone Aug 4, 2019

@googlebot googlebot added the cla: yes label Aug 4, 2019

@JoostK JoostK force-pushed the JoostK:ngcc-handle-diagnostics branch from 2ff361d to 00035a1 Aug 4, 2019

@JoostK JoostK marked this pull request as ready for review Aug 4, 2019

@JoostK JoostK requested a review from angular/fw-ngcc as a code owner Aug 4, 2019

@petebacondarwin
Copy link
Member

left a comment

One suggestion to avoid having to make up our own FormatDiagnosticHost.

packages/compiler-cli/ngcc/src/diagnostics.ts Outdated Show resolved Hide resolved
packages/compiler-cli/ngcc/src/main.ts Outdated Show resolved Hide resolved
@gkalpak
Copy link
Member

left a comment

😍

@JoostK JoostK force-pushed the JoostK:ngcc-handle-diagnostics branch 2 times, most recently from 8738754 to e8a2878 Aug 8, 2019

@JoostK

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

Blocked on a legit Codefresh failure. Instead of fixing it right away, we'll first make sure the emulated Windows FS mode behaves identical to the native Windows FS mode as to catch such failures earlier in the future.

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Good man @JoostK !!

@alxhub

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

@JoostK sounds good! I'm gonna remove merge-assistance so this doesn't get merged until the codefresh failure is fixed.

@JoostK JoostK force-pushed the JoostK:ngcc-handle-diagnostics branch from e8a2878 to 3ff6b7b Aug 11, 2019

@JoostK JoostK requested a review from angular/fw-compiler as a code owner Aug 11, 2019

@JoostK

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2019

Oh no! We don't currently have a Windows CI as Codefresh configuration was recently removed. I deliberately did not rebase my branch hoping that Codefresh would still run on the branch, but that doesn't appear to be the case :-(

@petebacondarwin I did add commit 3fdb4af which should resolve the discrepancy between emulated and native Windows mode. But unfortunately I can't test on native Windows now that Codefresh is gone...

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

@gkalpak is sometimes able to run things on his Windows setup...

// (see `MockFileSystemWindows`) so that the behavior is identical between native Windows and
// emulated Windows mode.
if (isWindows) {
path = path.replace(/^[\/\\]/i, 'C:/') as T;

This comment has been minimized.

Copy link
@gkalpak

gkalpak Aug 12, 2019

Member

Nit: The i flag is redundant (as is escaping / inside [...] 😁).

@gkalpak

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

As discussed "offline":

I ran yarn test-ivy-aot //packages/compiler-cli/ngcc/test/... //packages/compiler-cli/test/ngtsc and all tests passed (but ngcc/test:integration is skipped due to its no-ivy-aot tag).

Running yarn bazel test //packages/compiler-cli/ngcc/test/... //packages/compiler-cli/test/ngtsc gives the following failure for ngcc/test:integration:

<<FileSystem: Native>> ngcc main() diagnostics should fail with formatted diagnostics when an error diagnostic is produced
  Message:
    Expected function to throw an exception with message 'Failed to compile entry-point fatal-error due to compilation errors:
    node_modules/fatal-error/index.js(5,17): error TS-992001: component is missing a template
    ', but it threw an exception with message 'Failed to compile entry-point fatal-error due to compilation errors:
    node_modules/fatal-error/index.js(5,17): error TS-992001: component is missing a template
    '.

The difference being that the actual error ends with \r\n instead of \n. So, changing this assertion:

.toThrowError(
    'Failed to compile entry-point fatal-error due to compilation errors:\n' +
    'node_modules/fatal-error/index.js(5,17): error TS-992001: component is missing a template\n');

...to something like...

.toThrowError(
    'Failed to compile entry-point fatal-error due to compilation errors:\n' +
    `node_modules/fatal-error/index.js(5,17): error TS-992001: component is missing a template${require('os').EOL}`);

...makes all tests pass.

Interestingly, hard-coding \r\n in the above assertion, also makes all tests pass on Windows, which probably means that the mock-fs is doing something wrong (e.g. not mocking the system's EOL property, so that ts.formatDiagnostics() always uses the native EOL instead of the mocked one).

I am not sure what is the best way to solve this (e.g. normalize newlines in ts.formatDiagnostics() or fix the assertions, etc.). but normalizing newlines to \n on the output of ts.formatDiagnostics() sounds reasonable to me (assuming it does not break something else 😁).

@JoostK JoostK force-pushed the JoostK:ngcc-handle-diagnostics branch from 3ff6b7b to 78fdacc Aug 18, 2019

@mhevery

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Is this still blocked?

@mhevery

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@gkalpak

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

I just rerun the tests on Windows and they passed 👍

JoostK added 2 commits Aug 4, 2019
fix(ngcc): handle compilation diagnostics
Previously, any diagnostics reported during the compilation of an
entry-point would not be shown to the user, but either be ignored or
cause a hard crash in case of a `FatalDiagnosticError`. This is
unfortunate, as such error instances contain information on which code
was responsible for producing the error, whereas only its error message
would not. Therefore, it was quite hard to determine where the error
originates from.

This commit introduces behavior to deal with error diagnostics in a more
graceful way. Such diagnostics will still cause the compilation to fail,
however the error message now contains formatted diagnostics.

Closes #31977
Resolves FW-1374
test(ivy): normalize rooted paths to include a drive letter in Windows
The Angular compiler has an emulation system for various kinds of
filesystems and runs its testcases for all those filesystems. This
allows to verify that the compiler behaves correctly in all of the
supported platforms, without needing to run the tests on the actual
platforms.

Previously, the emulated Windows mode would normalize rooted paths to
always include a drive letter, whereas the native mode did not perform
this normalization. The consequence of this discrepancy was that running
the tests in native Windows was behaving differently compared to how
emulated Windows mode behaves, potentially resulting in test failures
in native Windows that would succeed for emulated Windows.

This commit adds logic to ensure that paths are normalized equally for
emulated Windows and native Windows mode, therefore resolving the
discrepancy.

@JoostK JoostK force-pushed the JoostK:ngcc-handle-diagnostics branch from 78fdacc to 504f500 Aug 29, 2019

@JoostK JoostK changed the title fix(ivy): ngcc - handle compilation diagnostics fix(ngcc): handle compilation diagnostics Aug 29, 2019

@JoostK

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

merge-assistance: job "legacy-unit-tests-saucelabs" is consistently timing out, unrelated to these changes.

@mhevery mhevery closed this in 4161d19 Aug 29, 2019

mhevery added a commit that referenced this pull request Aug 29, 2019
fix(ngcc): handle compilation diagnostics (#31996)
Previously, any diagnostics reported during the compilation of an
entry-point would not be shown to the user, but either be ignored or
cause a hard crash in case of a `FatalDiagnosticError`. This is
unfortunate, as such error instances contain information on which code
was responsible for producing the error, whereas only its error message
would not. Therefore, it was quite hard to determine where the error
originates from.

This commit introduces behavior to deal with error diagnostics in a more
graceful way. Such diagnostics will still cause the compilation to fail,
however the error message now contains formatted diagnostics.

Closes #31977
Resolves FW-1374

PR Close #31996
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
test(ivy): normalize rooted paths to include a drive letter in Windows (
angular#31996)

The Angular compiler has an emulation system for various kinds of
filesystems and runs its testcases for all those filesystems. This
allows to verify that the compiler behaves correctly in all of the
supported platforms, without needing to run the tests on the actual
platforms.

Previously, the emulated Windows mode would normalize rooted paths to
always include a drive letter, whereas the native mode did not perform
this normalization. The consequence of this discrepancy was that running
the tests in native Windows was behaving differently compared to how
emulated Windows mode behaves, potentially resulting in test failures
in native Windows that would succeed for emulated Windows.

This commit adds logic to ensure that paths are normalized equally for
emulated Windows and native Windows mode, therefore resolving the
discrepancy.

PR Close angular#31996
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
fix(ngcc): handle compilation diagnostics (angular#31996)
Previously, any diagnostics reported during the compilation of an
entry-point would not be shown to the user, but either be ignored or
cause a hard crash in case of a `FatalDiagnosticError`. This is
unfortunate, as such error instances contain information on which code
was responsible for producing the error, whereas only its error message
would not. Therefore, it was quite hard to determine where the error
originates from.

This commit introduces behavior to deal with error diagnostics in a more
graceful way. Such diagnostics will still cause the compilation to fail,
however the error message now contains formatted diagnostics.

Closes angular#31977
Resolves FW-1374

PR Close angular#31996
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.