fix(ngcc): bad package resilience #36083

petebacondarwin · 2020-03-16T11:34:11Z

Compiling @mat-datetimepicker/core : module as esm5
...
Error: Failed to compile entry-point @mat-datetimepicker/core (module as esm5) due to compilation errors:
...
Compiling ng-zorro-antd/message : module as esm5
Warning: Skipping processing of @mat-datetimepicker/moment because its dependency @mat-datetimepicker/core failed to compile.
Compiling @angular/flex-layout : module as esm5

petebacondarwin · 2020-03-16T21:53:49Z

I need to run a check against the CLI branch on ngcc-validation too.

gkalpak

I generally like this (with minor comments/suggestions) 👍
Also, great job spliting it up into cleanly separated commits 💯

Just a couple of thoughts/concerns:

I think I would prefer to have a consistent default value for errorOnFailedEntryPoint (false), regardless of how ngcc is called. I feel like it complicates things for little benefit otherwise. Especially given that changing the default based on programmatic call or not is orthogonal to the knowledge one has for the entry-points.
If anything, it might make sense to change the default value depending on whether the target option is set, but that would complicate things even more. (Even so, I would probably prefer this over the current behavior.)
I can be definitely convinced otherwise, though. What are your thoughts?
Previously, ngcc operated under the assumption that the found format properties would either be successfully compiled or the operation would halt altogether. This gave us the freedom to arbitrarily associate DTS processing with the first found format property. Now, this assumption is no longer true: It is theoretically possible that processing of es2015 for entry-point foo/bar (which has processDts: true) fails, but processing module for the same entry-point (with processDts: false) succeeds.
Should we rethink how we associate DTS processing with specific entry-point formats? (Note that this might complicate things significantly for parallel mode.)
Given the above and the fact that there is avery low probability for it to cause problems, I tend to believe it is better to keep operating under the current assumptions. We could also cancel/skip processing of all formats of an entry-point as soon as the task with processDts: true for that entry-point fails, but the extra compication is probably not worth it.

packages/compiler-cli/ngcc/src/execution/utils.ts

packages/compiler-cli/ngcc/main-ngcc.ts

packages/compiler-cli/ngcc/src/main.ts

packages/compiler-cli/ngcc/src/execution/task_selection/parallel_task_queue.ts

packages/compiler-cli/ngcc/test/execution/task_selection/parallel_task_queue_spec.ts

packages/compiler-cli/ngcc/src/execution/task_selection/base_task_queue.ts

packages/compiler-cli/ngcc/src/main.ts

JoostK

Excellent work!

I do agree with @gkalpak on the choice of the default value of the option, I'd prefer it to be always the same independent of the usage, or indeed be derived from targetedEntryPoint.

packages/compiler-cli/ngcc/src/execution/task_selection/base_task_queue.ts

packages/compiler-cli/ngcc/src/execution/utils.ts

packages/compiler-cli/ngcc/test/execution/helpers.ts

packages/compiler-cli/ngcc/test/execution/task_selection/serial_task_queue_spec.ts

petebacondarwin · 2020-03-18T14:37:08Z

Regarding the defaults. I have made both programmatic and command line version of errorOnFailedEntryPoint default to true.

This will mean that the CLI will need to add this option to the async CLI integration point to benefit from this.

I was not happy to make the default false since that would change the behaviour for the sync CLI integration point, where we definitely want the processing to fail if it comes up against a bad entry-point.

Furthermore, I don't believe that we should tie this errorOnFailedEntryPoint feature to whether we are targeting a specific package or not. They should be independent of each other.

petebacondarwin · 2020-03-18T14:57:59Z

Of course now the problem will be that the CLI needs to provide a command line option but it will fail if there is a version mismatch and the option is not yet available on the versions of Angular compiler-cli...

gkalpak

Some minor comments. Otherwise lgtm 👍

packages/compiler-cli/ngcc/test/execution/single_processor_executor_spec.ts

packages/compiler-cli/ngcc/src/execution/tasks/utils.ts

packages/compiler-cli/ngcc/src/execution/tasks/queues/parallel_task_queue.ts

packages/compiler-cli/ngcc/src/execution/tasks/completion.ts

packages/compiler-cli/ngcc/src/execution/tasks/queues/base_task_queue.ts

packages/compiler-cli/ngcc/src/writing/in_place_file_writer.ts

packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts

gkalpak · 2020-03-18T17:46:11Z

BTW, the 4th commit message needs updating wrt the current defaults for errorOnFailedEntryPoint.

This sets up the task execution to be able to report failed compiles

Moving the definition of the `onTaskCompleted` callback into `mainNgcc()` allows it to be configured based on options passed in there more easily. This will be the case when we want to configure whether to log or throw an error for tasks that failed to be processed successfully. This commit also creates two new folders and moves the code around a bit to make it easier to navigate the code§: * `execution/tasks`: specific helpers such as task completion handlers * `execution/tasks/queues`: the `TaskQueue` implementations and helpers

Later when we implement the ability to continue processing when tasks have failed to compile, we will also need to avoid processing tasks that depend upon the failed task. This refactoring exposes this list of dependent tasks in a way that can be used to skip processing of tasks that depend upon a failed task. It also changes the blocking model of the parallel mode of operation so that non-typings tasks are now blocked on their corresponding typings task. Previously the non-typings tasks could be triggered to run in parallel to the typings task, since they do not have a hard dependency on each other, but this made it difficult to skip task correctly if the typings task failed, since it was possible that a non-typings task was already in flight when the typings task failed. The result of this is a small potential degradation of performance in async parallel processing mode, in the rare cases that there were not enough unblocked tasks to make use of all the available workers.

gkalpak

LGTM 🎉 👌 💯

Commit message typo:

-This change adds a new `errorOnFailedEntryPoint` option to `mainNgcc` specifies whether ngcc should ...
+This change adds a new `errorOnFailedEntryPoint` option to `mainNgcc` that specifies whether ngcc should ...

packages/compiler-cli/ngcc/main-ngcc.ts

Previously, when an entry-point contained code that caused its compilation to fail, ngcc would exit in the middle of processing, possibly leaving other entry-points in a corrupt state. This change adds a new `errorOnFailedEntryPoint` option to `mainNgcc` that specifies whether ngcc should exit immediately or log an error and continue processing other entry-points. The default is `false` so that ngcc will not error but continue processing as much as possible. This is useful in post-install hooks, and async CLI integration, where we do not have as much control over which entry-points should be processed. The option is forced to true if the `targetEntryPointPath` is provided, such as the sync integration with the CLI, since in that case it is targeting an entry-point that will actually be used in the current project so we do want ngcc to exit with an error at that point.

When two entry-points overlap, ngcc may attempt to process some files twice. Previously, when this occured ngcc would just exit with an error preventing any other entry-points from being processed. This commit changes ngcc so that if `errorOnFailedEntryPoint` is false, it will simply log an error and continue to process entry-points. This is useful when ngcc is processing the entire node_modules folder and there are some invalid entry-points that the project doesn't actually use.

petebacondarwin · 2020-03-18T21:01:28Z

ngcc-validation is green : https://app.circleci.com/pipelines/github/angular/ngcc-validation/2438/workflows/9fdc3ecd-4984-4626-b3ee-8a10906df6da

Moving the definition of the `onTaskCompleted` callback into `mainNgcc()` allows it to be configured based on options passed in there more easily. This will be the case when we want to configure whether to log or throw an error for tasks that failed to be processed successfully. This commit also creates two new folders and moves the code around a bit to make it easier to navigate the code§: * `execution/tasks`: specific helpers such as task completion handlers * `execution/tasks/queues`: the `TaskQueue` implementations and helpers PR Close #36083

…36083) Later when we implement the ability to continue processing when tasks have failed to compile, we will also need to avoid processing tasks that depend upon the failed task. This refactoring exposes this list of dependent tasks in a way that can be used to skip processing of tasks that depend upon a failed task. It also changes the blocking model of the parallel mode of operation so that non-typings tasks are now blocked on their corresponding typings task. Previously the non-typings tasks could be triggered to run in parallel to the typings task, since they do not have a hard dependency on each other, but this made it difficult to skip task correctly if the typings task failed, since it was possible that a non-typings task was already in flight when the typings task failed. The result of this is a small potential degradation of performance in async parallel processing mode, in the rare cases that there were not enough unblocked tasks to make use of all the available workers. PR Close #36083

Previously, when an entry-point contained code that caused its compilation to fail, ngcc would exit in the middle of processing, possibly leaving other entry-points in a corrupt state. This change adds a new `errorOnFailedEntryPoint` option to `mainNgcc` that specifies whether ngcc should exit immediately or log an error and continue processing other entry-points. The default is `false` so that ngcc will not error but continue processing as much as possible. This is useful in post-install hooks, and async CLI integration, where we do not have as much control over which entry-points should be processed. The option is forced to true if the `targetEntryPointPath` is provided, such as the sync integration with the CLI, since in that case it is targeting an entry-point that will actually be used in the current project so we do want ngcc to exit with an error at that point. PR Close #36083

When two entry-points overlap, ngcc may attempt to process some files twice. Previously, when this occured ngcc would just exit with an error preventing any other entry-points from being processed. This commit changes ngcc so that if `errorOnFailedEntryPoint` is false, it will simply log an error and continue to process entry-points. This is useful when ngcc is processing the entire node_modules folder and there are some invalid entry-points that the project doesn't actually use. PR Close #36083

angular-automatic-lock-bot · 2020-04-19T16:05:40Z

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.}

petebacondarwin added type: bug/fix target: major This PR is targeted for the next major release comp: ngcc labels Mar 16, 2020

ngbot bot modified the milestone: needsTriage Mar 16, 2020

googlebot added the cla: yes label Mar 16, 2020

petebacondarwin force-pushed the ngcc-bad-package-resilience branch 7 times, most recently from 08ff318 to fb73362 Compare March 16, 2020 17:33

petebacondarwin marked this pull request as ready for review March 16, 2020 17:40

pullapprove bot requested a review from JoostK March 16, 2020 17:40

petebacondarwin requested review from gkalpak and JoostK and removed request for JoostK March 16, 2020 17:40

petebacondarwin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 16, 2020

petebacondarwin force-pushed the ngcc-bad-package-resilience branch from fb73362 to 5abfd11 Compare March 16, 2020 20:43

gkalpak requested changes Mar 17, 2020

View reviewed changes

kara 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 Mar 17, 2020

kara assigned petebacondarwin Mar 17, 2020

JoostK approved these changes Mar 17, 2020

View reviewed changes

petebacondarwin force-pushed the ngcc-bad-package-resilience branch 5 times, most recently from fd991ae to 902eb1e Compare March 18, 2020 14:03

petebacondarwin force-pushed the ngcc-bad-package-resilience branch from 902eb1e to 2d4b6b3 Compare March 18, 2020 14:33

petebacondarwin force-pushed the ngcc-bad-package-resilience branch from 2d4b6b3 to ea416e9 Compare March 18, 2020 14:41

petebacondarwin removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 18, 2020

gkalpak requested changes Mar 18, 2020

View reviewed changes

petebacondarwin force-pushed the ngcc-bad-package-resilience branch from ea416e9 to 4939c26 Compare March 18, 2020 17:52

petebacondarwin added 3 commits March 18, 2020 18:41

refactor(ngcc): add message text to task outcomes

66ee90c

This sets up the task execution to be able to report failed compiles

petebacondarwin force-pushed the ngcc-bad-package-resilience branch 2 times, most recently from bf0f800 to 408714f Compare March 18, 2020 20:40

gkalpak approved these changes Mar 18, 2020

View reviewed changes

packages/compiler-cli/ngcc/main-ngcc.ts Outdated Show resolved Hide resolved

petebacondarwin added 2 commits March 18, 2020 20:47

petebacondarwin force-pushed the ngcc-bad-package-resilience branch from 408714f to 4106aca Compare March 18, 2020 21:00

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 18, 2020

JoostK approved these changes Mar 18, 2020

View reviewed changes

AndrewKushnir closed this in 712f264 Mar 18, 2020

petebacondarwin deleted the ngcc-bad-package-resilience branch March 19, 2020 09:22

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngcc): bad package resilience #36083

fix(ngcc): bad package resilience #36083

petebacondarwin commented Mar 16, 2020 •

edited

petebacondarwin commented Mar 16, 2020

gkalpak left a comment

JoostK left a comment

petebacondarwin commented Mar 18, 2020

petebacondarwin commented Mar 18, 2020

gkalpak left a comment

gkalpak commented Mar 18, 2020

gkalpak left a comment

petebacondarwin commented Mar 18, 2020

angular-automatic-lock-bot bot commented Apr 19, 2020

fix(ngcc): bad package resilience #36083

fix(ngcc): bad package resilience #36083

Conversation

petebacondarwin commented Mar 16, 2020 • edited

petebacondarwin commented Mar 16, 2020

gkalpak left a comment

Choose a reason for hiding this comment

JoostK left a comment

Choose a reason for hiding this comment

petebacondarwin commented Mar 18, 2020

petebacondarwin commented Mar 18, 2020

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak commented Mar 18, 2020

gkalpak left a comment

Choose a reason for hiding this comment

petebacondarwin commented Mar 18, 2020

angular-automatic-lock-bot bot commented Apr 19, 2020

petebacondarwin commented Mar 16, 2020 •

edited