ngcc: add build marker by petebacondarwin · Pull Request #25557 · angular/angular

petebacondarwin · 2018-08-17T21:03:20Z

This sits on top of #25406

googlebot · 2018-08-17T21:03:40Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

mary-poppins · 2018-08-17T21:10:23Z

You can preview 03b6121 at https://pr25557-03b6121.ngbuilds.io/.

gkalpak

AFAICT, more logic is needed. E.g. when we detect that a package has been compiled by a different ngcc version.
I.e. we need a way to "uncompile" before compiling with the new version. ("Uncompiling" could be as simple as rimrafing node_modules_ngtsc/ or as complex as walking node_modules/ and renaming all backup files to their original name.)

I think we also need to record the compilations output path (e.g. node_modules/ or node_modules_ngtsc/) in the marker metadata.

gkalpak · 2018-08-20T19:11:59Z

Couldn't we use @angular/compiler-cli VERSION instead?

I would rather not have a dependency on compiler-cli if we can avoid it. That version is created in exactly the same way as this anyway.

Hm...I thought ngcc will be part of the compiler-cli package anyway.
In any case, not a big deal 😁

To give more strength to this approach I can also say that I conferred with Alex Eagle about the best approach here as well :-)

mary-poppins · 2018-08-21T08:49:09Z

You can preview a616204 at https://pr25557-a616204.ngbuilds.io/.

petebacondarwin · 2018-08-21T08:49:57Z

We should avoid trying to "uncompile" at this stage. The backups are there if we need them for adding such a strategy later. In the meantime I will add a message telling the user to blow away their node_modules if the ngcc version changes.

mary-poppins · 2018-08-22T08:53:28Z

You can preview a35ec67 at https://pr25557-a35ec67.ngbuilds.io/.

mary-poppins · 2018-08-22T11:08:35Z

You can preview 02c2f33 at https://pr25557-02c2f33.ngbuilds.io/.

googlebot · 2018-08-23T08:40:14Z

CLAs look good, thanks!

mary-poppins · 2018-08-23T08:48:57Z

You can preview a6e05a5 at https://pr25557-a6e05a5.ngbuilds.io/.

petebacondarwin · 2018-08-23T11:37:24Z

We just need to work out what to do about the failing integration test: bazel tests do not allow the subject to write over the node_modules of the test folder.

`ngcc` adds marker files to each folder that has been compiled, containing the version of the ngcc used. When compiling, it will ignore folders that contain these marker files, as long as the version matches.

Bazel does not like the filesystem being modified. This commit a temporary mock filesystem that can be modified as needed.

mary-poppins · 2018-08-28T10:41:06Z

You can preview 137a198 at https://pr25557-137a198.ngbuilds.io/.

mary-poppins · 2018-08-28T10:42:26Z

You can preview 736507f at https://pr25557-736507f.ngbuilds.io/.

petebacondarwin · 2018-08-28T10:46:14Z

I have implemented an (in-memory) mock filesystem for the integration test that was failing.
This allows the test to be able to write to the node_modules folder safely. It could also allow
us to do more subtle testing of file changes at this level, going forward.

petebacondarwin · 2018-09-02T13:05:31Z

Awaiting G3 resubmit and pullapprove for integration test.
^^^ @alxhub or @mhevery

Bazel does not like the filesystem being modified. This commit a temporary mock filesystem that can be modified as needed. PR Close #25557

angular-automatic-lock-bot · 2019-09-14T00:52:21Z

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 action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Aug 17, 2018

petebacondarwin added this to the Ivy milestone Aug 17, 2018

petebacondarwin requested a review from alxhub August 17, 2018 21:03

googlebot added the cla: no label Aug 17, 2018

gkalpak reviewed Aug 20, 2018

View reviewed changes

petebacondarwin force-pushed the ngcc-build-marker branch from 03b6121 to a616204 Compare August 21, 2018 08:41

petebacondarwin force-pushed the ngcc-build-marker branch from a616204 to a35ec67 Compare August 22, 2018 08:43

petebacondarwin force-pushed the ngcc-build-marker branch from 02c2f33 to a6e05a5 Compare August 23, 2018 08:40

googlebot added cla: yes and removed cla: no labels Aug 23, 2018

alxhub approved these changes Aug 27, 2018

View reviewed changes

petebacondarwin added 2 commits August 28, 2018 09:43

feat(ivy): implement ngcc build marker

3b721e4

`ngcc` adds marker files to each folder that has been compiled, containing the version of the ngcc used. When compiling, it will ignore folders that contain these marker files, as long as the version matches.

fixup! feat(ivy): implement ngcc build marker

e702858

petebacondarwin force-pushed the ngcc-build-marker branch from a6e05a5 to 137a198 Compare August 28, 2018 10:33

test(ivy): mock the filesystem in ngcc main() integration tests

736507f

Bazel does not like the filesystem being modified. This commit a temporary mock filesystem that can be modified as needed.

petebacondarwin force-pushed the ngcc-build-marker branch from 137a198 to 736507f Compare August 28, 2018 10:34

alxhub 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 Sep 5, 2018

mhevery closed this in b0cb134 Sep 5, 2018

mhevery pushed a commit that referenced this pull request Sep 5, 2018

test(ivy): mock the filesystem in ngcc main() integration tests (#25557)

d96e962

Bazel does not like the filesystem being modified. This commit a temporary mock filesystem that can be modified as needed. PR Close #25557

petebacondarwin deleted the ngcc-build-marker branch September 6, 2018 07:38

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 14, 2019

Conversation

petebacondarwin commented Aug 17, 2018

Uh oh!

googlebot commented Aug 17, 2018

Uh oh!

mary-poppins commented Aug 17, 2018

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

gkalpak Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

gkalpak Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Aug 21, 2018

Uh oh!

petebacondarwin commented Aug 21, 2018

Uh oh!

mary-poppins commented Aug 22, 2018

Uh oh!

mary-poppins commented Aug 22, 2018

Uh oh!

googlebot commented Aug 23, 2018

Uh oh!

mary-poppins commented Aug 23, 2018

Uh oh!

petebacondarwin commented Aug 23, 2018

Uh oh!

mary-poppins commented Aug 28, 2018

Uh oh!

mary-poppins commented Aug 28, 2018

Uh oh!

petebacondarwin commented Aug 28, 2018

Uh oh!

petebacondarwin commented Sep 2, 2018

Uh oh!

angular-automatic-lock-bot Bot commented Sep 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants