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

βœ¨πŸ— Upgrade closure compiler to v20190301 #21618

Merged
merged 60 commits into from
Apr 10, 2019
Merged

βœ¨πŸ— Upgrade closure compiler to v20190301 #21618

merged 60 commits into from
Apr 10, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Mar 28, 2019

This PR upgrades the version of closure compiler used to minify the AMP runtime to v20190301.

Highlights:

For posterity, here is a list of things I've reviewed / verified / tested.

Code review:

  • Self-reviewed all 240+ files in this PR to make sure there are no stray changes
  • Re-read all 200+ review comments to make sure they were satisfactorily addressed and resolved
  • Filed issues for future clean up tasks / feature requests

Local tests:

  • Ran gulp check-types to make sure there are no type errors
  • Ran gulp dist --fortesting to make sure minified multi-pass build is healthy
    • Ran gulp serve --compiled and manually tested several components in multi-pass mode
  • Ran gulp dist --fortesting --single_pass --pseudo_names to make sure minified single-pass build is healthy
    • Ran gulp serve --compiled and manually tested several components in single-pass mode
  • Ran gulp test --local-changes --headless to make sure all unit tests affected by this PR are green

Travis tests:

  • Checks (presubmit, lint, dep-check, and check-types) are green (link)
  • Un-minified build is green (link)
  • Visual tests are green with zero diffs due to this PR (link)
  • All unit tests are green (link)
  • All integration tests are green (link)
  • Minified single-pass build and tests are green (link)

AMP Release schedule

  • AMP Conference 2019 is next week. We're doing a release on Tuesday, Apr 16 using this week's canary cut, based on the 1904091426070 tag. The release will be unaffected by this upgrade.

Partial fix for #18748
Follow up to #18552
Follow up to #18609
Follow up to #18794
Follow up to #19449
Follow up to #20056

@rsimha rsimha changed the title βœ¨πŸ— [WIP] Upgrade closure compiler to v20190301 βœ¨πŸ— Upgrade closure compiler to v20190301 Mar 28, 2019
@ampproject ampproject deleted a comment Mar 28, 2019
@rsimha rsimha self-assigned this Mar 28, 2019
@rsimha rsimha requested a review from jridgewell March 28, 2019 23:40
@rsimha
Copy link
Contributor Author

rsimha commented Mar 28, 2019

At long last, this PR is ready for preliminary review. We are now at a point where gulp dist passes in multi-pass and single-pass mode, and gulp check-types, gulp lint, and gulp presubmit are all green. I'm still manually testing the changes I've made, and working through test failures.

Sending this out to all TLs and those familiar with closure compiler. Feel free to assign directories / files to others on your team. For easy reviewing, I've split off the changes into separate commits as far as possible.

@cramforce
Copy link
Member

Woohoo! Makes me super happy that we are so close to head!

@rsimha
Copy link
Contributor Author

rsimha commented Mar 29, 2019

Good news: Build looks good in multi-pass mode with all tests passing (link) and no visual diffs (link)
Bad news: Single pass build is busted, with several test failures (link) Edit: This is now fixed. See latest comments.

Paging @erwinmombay to see if I'm missing something obvious that's causing the single-pass failures. Manual testing confirms that the single-pass build isn't working.

Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Left a few questions while Single Pass is being worked on.

Great work @rsimha!

@rsimha
Copy link
Contributor Author

rsimha commented Mar 29, 2019

@kristoferbaxter Thanks for keeping me honest with your comments. I've left responses to each of them, and will let you resolve them when you're satisfied that there's no better alternative.

Now on to dealing with the single-pass errors...

Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Toooo many files...

extensions/amp-ad-exit/0.1/amp-ad-exit.js Outdated Show resolved Hide resolved
extensions/amp-date-display/0.1/amp-date-display.js Outdated Show resolved Hide resolved
extensions/amp-form/0.1/amp-form-textarea.js Outdated Show resolved Hide resolved
extensions/amp-form/0.1/amp-form-textarea.js Outdated Show resolved Hide resolved
extensions/amp-iframe/0.1/amp-iframe.js Show resolved Hide resolved
extensions/amp-addthis/0.1/config-manager.js Outdated Show resolved Hide resolved
extensions/amp-animation/0.1/amp-animation.js Outdated Show resolved Hide resolved
extensions/amp-apester-media/0.1/amp-apester-media.js Outdated Show resolved Hide resolved
extensions/amp-autocomplete/0.1/amp-autocomplete.js Outdated Show resolved Hide resolved
extensions/amp-lightbox/0.1/amp-lightbox.js Show resolved Hide resolved
Copy link
Contributor Author

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Addressed all comments by @jridgewell.

extensions/amp-ad-exit/0.1/amp-ad-exit.js Outdated Show resolved Hide resolved
extensions/amp-addthis/0.1/amp-addthis.js Outdated Show resolved Hide resolved
extensions/amp-addthis/0.1/amp-addthis.js Show resolved Hide resolved
extensions/amp-analytics/0.1/amp-analytics.js Outdated Show resolved Hide resolved
extensions/amp-form/0.1/amp-form-textarea.js Outdated Show resolved Hide resolved
extensions/amp-date-display/0.1/amp-date-display.js Outdated Show resolved Hide resolved
extensions/amp-date-display/0.1/amp-date-display.js Outdated Show resolved Hide resolved
extensions/amp-form/0.1/amp-form-textarea.js Outdated Show resolved Hide resolved
extensions/amp-iframe/0.1/amp-iframe.js Show resolved Hide resolved
@rsimha rsimha requested a review from aghassemi April 10, 2019 15:59
@rsimha
Copy link
Contributor Author

rsimha commented Apr 10, 2019

The canary cut for the AMP Conf release has been pushed as the 1904091426070 tag. It is now safe to merge this PR. (/cc @choumx who is release on-duty this week)

Once @aghassemi and @newmuis approve the final changes, I'll do another full round of testing and then merge this PR.

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

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

Thanks for all the work here to move this forward.

@rsimha
Copy link
Contributor Author

rsimha commented Apr 10, 2019

This PR is now ready to merge. For posterity, here is a list of things I've reviewed / verified / tested.

Code review:

  • Self-reviewed all 240+ files in this PR to make sure there are no stray changes
  • Re-read all 200+ review comments to make sure they were satisfactorily addressed and resolved
  • Filed issues for future clean up tasks / feature requests

Local tests:

  • Ran gulp check-types to make sure there are no type errors
  • Ran gulp dist --fortesting to make sure minified multi-pass build is healthy
    • Ran gulp serve --compiled and manually tested several components in multi-pass mode
  • Ran gulp dist --fortesting --single_pass --pseudo_names to make sure minified single-pass build is healthy
    • Ran gulp serve --compiled and manually tested several components in single-pass mode
  • Ran gulp test --local-changes --headless to make sure all unit tests affected by this PR are green

Travis tests:

  • Checks (presubmit, lint, dep-check, and check-types) are green (link)
  • Un-minified build is green (link)
  • Visual tests are green with zero diffs due to this PR (link)
  • All unit tests are green (link)
  • All integration tests are green (link)
  • Minified single-pass build and tests are green (link)

AMP Release schedule

  • AMP Conference 2019 is next week. We're doing a release on Tuesday, Apr 16 using this week's canary cut, which was based on the 1904091426070 tag.
  • This PR was merged after the AMP Conf release tag, so the main AMP Conf release is unaffected by any of these changes.

@rsimha rsimha merged commit 680dd18 into ampproject:master Apr 10, 2019
@rsimha rsimha deleted the 2019-03-22-ClosureCompiler branch April 10, 2019 19:16
@rsimha
Copy link
Contributor Author

rsimha commented Apr 10, 2019

image

@dvoytenko
Copy link
Contributor

@rsimha Probably needs to be rolled back :( It updated the third_party code improperly. E.g. third_party/config/config.js cannot do dev().assert.

@rsimha
Copy link
Contributor Author

rsimha commented Apr 11, 2019

@dvoytenko Thanks for your comment. We can revert the changes to third_party/, as you've done in #21827. For an explanation of why I (incorrectly) made the change, see #21827 (review).

I'll approve and merge your partial revert ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants