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): implement lockfile #34722

Closed
wants to merge 5 commits into from

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Jan 10, 2020

Previously, it was possible for multiple instance of ngcc to be running
at the same time, but this is not supported and can cause confusing and
flakey errors at build time.

Now, only one instance of ngcc can run at a time. If a second instance
tries to execute it fails with an appropriate error message.

See #32431 (comment)

@googlebot googlebot added the cla: yes label Jan 10, 2020
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-lockfile branch from 6519660 to ee60c99 Jan 10, 2020
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-lockfile branch from ee60c99 to 9a37183 Jan 13, 2020
@petebacondarwin petebacondarwin changed the title Ngcc lockfile fix(ngcc): implement lockfile Jan 13, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jan 13, 2020
@petebacondarwin petebacondarwin marked this pull request as ready for review Jan 13, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jan 13, 2020
@petebacondarwin petebacondarwin requested review from angular/fw-compiler as code owners Jan 13, 2020
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-lockfile branch from 9a37183 to a7f3eed Jan 13, 2020
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-lockfile branch from a7f3eed to 023a995 Jan 13, 2020
packages/compiler-cli/ngcc/src/execution/lock_file.ts Outdated Show resolved Hide resolved
packages/compiler-cli/ngcc/src/execution/lock_file.ts Outdated Show resolved Hide resolved
process.once('SIGINT', this.signalHandler);
process.once('SIGHUP', this.signalHandler);
Comment on lines +75 to +94

This comment has been minimized.

Copy link
@JoostK

JoostK Jan 14, 2020

Member

OOC: Does this work on Windows?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Jan 15, 2020

Author Member

No idea! I am hoping that our CI (or one of our Windows gurus) will test that for me :-)

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Jan 15, 2020

Author Member

But to be serious, I looked into it and in theory this should be enough...

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-lockfile branch 2 times, most recently from 2f60cef to 2157c9b Jan 15, 2020
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-lockfile branch from 2157c9b to 9ccf5a4 Jan 16, 2020
Copy link
Member

gkalpak left a comment

Looking good! Some minor comments/questions/suggestions.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-lockfile branch from 6456953 to 673e4a3 Jan 17, 2020
@petebacondarwin petebacondarwin requested a review from angular/framework-global-approvers-for-docs-only-changes as a code owner Jan 17, 2020
@petebacondarwin petebacondarwin requested review from gkalpak and JoostK Jan 17, 2020
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-lockfile branch from 673e4a3 to f16e7e4 Jan 17, 2020
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-lockfile branch from d09b338 to 2ce082f Jan 17, 2020
Copy link
Member

gkalpak left a comment

👌

@JoostK
JoostK approved these changes Jan 20, 2020
@matsko

This comment has been minimized.

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jan 22, 2020

@matsko - what is the reason for the presubmit failure?

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Jan 22, 2020

@petebacondarwin it looks like there were some pre-existing failing tests. I marked "google3" check as passing for this PR. Thank you.

@matsko

This comment has been minimized.

Copy link
Member

matsko commented Jan 22, 2020

@petebacondarwin please rebase.

This commit adds an `exclusive` parameter to the
`FileSystem.writeFile()` method. When this parameter is
true, the method will fail with an `EEXIST` error if the
file already exists on disk.
Previously, it was possible for multiple instance of ngcc to be running
at the same time, but this is not supported and can cause confusing and
flakey errors at build time.

Now, only one instance of ngcc can run at a time. If a second instance
tries to execute it fails with an appropriate error message.

See #32431 (comment)
The Angular CLI will continue to call ngcc on all possible packages, even if they
have already been processed by ngcc in a postinstall script.

In a parallel build environment, this was causing ngcc to complain that it was
being run in more than one process at the same time.

This commit moves the check for whether the targeted package has been
processed outside the locked code section, since there is no issue with
multiple ngcc processes from doing this check.
Similar to c602563 this commit aligns the node.js version
requirements of the Angular compiler with Angular CLI.
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-lockfile branch from 84bcaee to 7a644ef Jan 22, 2020
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jan 22, 2020

@matsko - done.

AndrewKushnir added a commit that referenced this pull request Jan 22, 2020
This commit adds an `exclusive` parameter to the
`FileSystem.writeFile()` method. When this parameter is
true, the method will fail with an `EEXIST` error if the
file already exists on disk.

PR Close #34722
AndrewKushnir added a commit that referenced this pull request Jan 22, 2020
Previously, it was possible for multiple instance of ngcc to be running
at the same time, but this is not supported and can cause confusing and
flakey errors at build time.

Now, only one instance of ngcc can run at a time. If a second instance
tries to execute it fails with an appropriate error message.

See #32431 (comment)

PR Close #34722
AndrewKushnir added a commit that referenced this pull request Jan 22, 2020
The Angular CLI will continue to call ngcc on all possible packages, even if they
have already been processed by ngcc in a postinstall script.

In a parallel build environment, this was causing ngcc to complain that it was
being run in more than one process at the same time.

This commit moves the check for whether the targeted package has been
processed outside the locked code section, since there is no issue with
multiple ngcc processes from doing this check.

PR Close #34722
AndrewKushnir added a commit that referenced this pull request Jan 22, 2020
Similar to c602563 this commit aligns the node.js version
requirements of the Angular compiler with Angular CLI.

PR Close #34722
AndrewKushnir added a commit that referenced this pull request Jan 22, 2020
This commit adds an `exclusive` parameter to the
`FileSystem.writeFile()` method. When this parameter is
true, the method will fail with an `EEXIST` error if the
file already exists on disk.

PR Close #34722
AndrewKushnir added a commit that referenced this pull request Jan 22, 2020
Previously, it was possible for multiple instance of ngcc to be running
at the same time, but this is not supported and can cause confusing and
flakey errors at build time.

Now, only one instance of ngcc can run at a time. If a second instance
tries to execute it fails with an appropriate error message.

See #32431 (comment)

PR Close #34722
AndrewKushnir added a commit that referenced this pull request Jan 22, 2020
The Angular CLI will continue to call ngcc on all possible packages, even if they
have already been processed by ngcc in a postinstall script.

In a parallel build environment, this was causing ngcc to complain that it was
being run in more than one process at the same time.

This commit moves the check for whether the targeted package has been
processed outside the locked code section, since there is no issue with
multiple ngcc processes from doing this check.

PR Close #34722
AndrewKushnir added a commit that referenced this pull request Jan 22, 2020
Similar to c602563 this commit aligns the node.js version
requirements of the Angular compiler with Angular CLI.

PR Close #34722
@petebacondarwin petebacondarwin deleted the petebacondarwin:ngcc-lockfile branch Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.