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): updates ngc to pass the build when only warnings are emitted #43673

Closed
wants to merge 1 commit into from

Conversation

dgp1130
Copy link
Contributor

@dgp1130 dgp1130 commented Oct 1, 2021

Refs #42966.

Previously, a build when emitted one warning and no errors would fail with a non-zero exit code. This is not what users would expect, but had not been an issue before since the compiler did not actually emit any warnings. With upcoming extended template diagnostics and other warnings, this is now a case that needs to be supported. Warnings are printed to stderr as before, but ngc now exits with code 0 and the build is considered successful.

Implemented this by adding a new driveMainWithWarnings() test method which returns the exit code and any messages logged to stderr. Most importantly, it does not require the the build to pass, so it is up to the test to assert this as well as many messages printed to make sure they are acceptable. This is useful for testing warnings and ensuring the build still passes.

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: #42966.

ngc fails when a compilation returns warnings but no errors. It should succeed as long as no errors are emitted, regardless of any warnings.

What is the new behavior?

ngc now exits with code 0 (passes) when a compilation returns no errors, even if warnings are emitted.

Does this PR introduce a breaking change?

  • No

The compiler does not currently produce warnings, so there is no change in behavior. This is intended to unblock future compiler changes.

Other information

/cc @atscott.

@dgp1130 dgp1130 added the target: patch This PR is targeted for the next patch release label Oct 1, 2021
@dgp1130 dgp1130 requested a review from alxhub October 1, 2021 22:43
@google-cla google-cla bot added the cla: yes label Oct 1, 2021
@dylhunn dylhunn added the area: compiler Issues related to `ngc`, Angular's template compiler label Oct 4, 2021
@ngbot ngbot bot added this to the Backlog milestone Oct 4, 2021
@dgp1130
Copy link
Contributor Author

dgp1130 commented Oct 5, 2021

@alxhub is this required for v13 so ngc doesn't fail with the new deprecation warning?

packages/compiler-cli/test/ngtsc/env.ts Outdated Show resolved Hide resolved
@dgp1130
Copy link
Contributor Author

dgp1130 commented Oct 6, 2021

@alxhub is this required for v13 so ngc doesn't fail with the new deprecation warning?

Chatted in Slack, the deprecating warning won't make v13 anyways so no need to get this in.

… are emitted

Refs angular#42966.

Previously, a build when emitted one warning and no errors would fail with a non-zero exit code. This is not what users would expect, but had not been an issue before since the compiler did not actually emit any warnings. With upcoming extended template diagnostics and other warnings, this is now a case that needs to be supported. Warnings are printed to `stderr` as before, but `ngc` now exits with code `0` and the build is considered successful.

Implemented this by adding a new `expectedExitCode` parameter to `driveDiagnostics()` which asserts against the real exit code. Most importantly, it does not **require** the the build to pass since any exit code can be given, so it is up to the test to assert this as well as many messages printed to make sure they are acceptable. This is useful for testing warnings and ensuring the build still passes.
@dgp1130 dgp1130 added the action: merge The PR is ready for merge by the caretaker label Oct 15, 2021
@AndrewKushnir AndrewKushnir added target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release labels Oct 15, 2021
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit b2b5972.

AndrewKushnir pushed a commit that referenced this pull request Oct 15, 2021
… are emitted (#43673)

Refs #42966.

Previously, a build when emitted one warning and no errors would fail with a non-zero exit code. This is not what users would expect, but had not been an issue before since the compiler did not actually emit any warnings. With upcoming extended template diagnostics and other warnings, this is now a case that needs to be supported. Warnings are printed to `stderr` as before, but `ngc` now exits with code `0` and the build is considered successful.

Implemented this by adding a new `expectedExitCode` parameter to `driveDiagnostics()` which asserts against the real exit code. Most importantly, it does not **require** the the build to pass since any exit code can be given, so it is up to the test to assert this as well as many messages printed to make sure they are acceptable. This is useful for testing warnings and ensuring the build still passes.

PR Close #43673
@dgp1130 dgp1130 deleted the ngc-diagnostics branch October 15, 2021 17:18
@AndrewKushnir
Copy link
Contributor

@dgp1130 there was a merge conflict with the 12.2.x branch, so I merged the change to master and 13.0.x. If this change needs to land in 12.2.x, could you please create a new PR with the necessary changes? Thank you.

@dgp1130
Copy link
Contributor Author

dgp1130 commented Oct 15, 2021

@AndrewKushnir, considering 12.2.x will be falling into LTS in a few weeks, I think we can just drop it. Only reason was to keep the codebase consistent, but I don't think that's important enough to out of our way for here.

@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 Nov 15, 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: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants