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 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)

@petebacondarwin petebacondarwin changed the title Ngcc lockfile fix(ngcc): implement lockfile Jan 13, 2020
@petebacondarwin petebacondarwin added comp: ngcc action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release and removed state: WIP labels Jan 13, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jan 13, 2020
@petebacondarwin petebacondarwin marked this pull request as ready for review January 13, 2020 08:01
@ngbot ngbot bot added this to the needsTriage milestone Jan 13, 2020
@petebacondarwin petebacondarwin requested review from a team as code owners January 13, 2020 08:01
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
Comment on lines +75 to +94
process.once('SIGINT', this.signalHandler);
process.once('SIGHUP', this.signalHandler);
Copy link
Member

Choose a reason for hiding this comment

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

OOC: Does this work on Windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@petebacondarwin petebacondarwin force-pushed the ngcc-lockfile branch 2 times, most recently from 2f60cef to 2157c9b Compare January 16, 2020 08:21
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Looking good! Some minor comments/questions/suggestions.

@petebacondarwin
Copy link
Member Author

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

@petebacondarwin petebacondarwin removed the action: presubmit The PR is in need of a google3 presubmit label Jan 22, 2020
@AndrewKushnir
Copy link
Contributor

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

@matsko
Copy link
Contributor

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 angular#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
Copy link
Member Author

@matsko - done.

AndrewKushnir pushed 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 pushed 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 pushed 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 pushed 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 pushed a commit that referenced this pull request Jan 22, 2020
AndrewKushnir pushed 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 pushed 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 pushed 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 pushed 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 ngcc-lockfile branch January 23, 2020 08:54
jeripeierSBB pushed a commit to sbb-design-systems/sbb-angular that referenced this pull request Jan 30, 2020
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 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 angular#34722
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 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 angular#32431 (comment)

PR Close angular#34722
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020
…4722)

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 angular#34722
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020
Similar to c602563 this commit aligns the node.js version
requirements of the Angular compiler with Angular CLI.

PR Close angular#34722
@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 Feb 23, 2020
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 cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants