Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

build: make the build output deterministic and reproducible #11570

Merged
merged 1 commit into from
Mar 13, 2019

Conversation

Splaktar
Copy link
Contributor

@Splaktar Splaktar commented Dec 29, 2018

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[x] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Consecutive builds, without any changes, result in bundle content with different ordering. This is even true in a single build where the dist/angular-material.js isn't identical to the dist/docs/angular-material.js. This makes comparing releases harder and it makes syncing into g3 more time consuming.

  • passing globs to gulp.src can cause ordering to not be preserved.
    this is because gulp.src is async
  • gulp-concat tries to preserve the order passed into gulp.src.
    if the globbing is too vague, then there isn't enough info for ordering

Issue Number:
Fixes #11502

What is the new behavior?

All of the build output (JS, SCSS, CSS) should be identical if the build is run multiple times without any changes to the code.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

  • more statically defining the paths like this isn't ideal but it is acceptable atm since we aren't adding new components.

I looked at sorting the file paths manually or using gulp-sort but neither solution was flexible enough to order some of our internal dependencies properly. This approach seems to work fine w/o adding gulp-order, but if problems pop up in the future, that is another package that might help.

Note that the root cause of this lies in the intersection of gulp.src in Gulp core and gulp-concat. This is a well established problem that the Gulp team has mostly decided to punt into user land or third party packages so that they can maintain speed and not need to support crazy levels of customization.

@Splaktar Splaktar added this to the 1.1.12 milestone Dec 29, 2018
@Splaktar Splaktar self-assigned this Dec 29, 2018
@Splaktar Splaktar requested a review from jelbourn December 29, 2018 00:11
@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Dec 29, 2018
@Splaktar Splaktar added type: build P3: important Important issues that really should be fixed when possible. labels Dec 29, 2018
@Splaktar Splaktar added the pr: merge ready This PR is ready for a caretaker to review label Dec 29, 2018
@Splaktar Splaktar modified the milestones: 1.1.12, 1.1.13 Jan 3, 2019
@Splaktar Splaktar assigned andrewseguin and unassigned jelbourn Jan 7, 2019
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn
Copy link
Member

jelbourn commented Jan 9, 2019

Caretaker: this PR should be synced on its own. It should be a complete no-op, but on the off chance that something does break, we'll know it's from this and not another PR.

@Splaktar Splaktar assigned mmalerba and unassigned andrewseguin Jan 29, 2019
@Splaktar Splaktar force-pushed the gulp-deterministicBuilds branch from bc4cbf9 to 944994b Compare January 31, 2019 06:07
.pipe(utils.cssToNgConstant('material.core', '$MD_THEME_CSS'));
var paths = config.themeBaseFiles;
config.componentPaths.forEach(component => paths.push(path.join(component, '*-theme.scss')));
paths.push(config.themeCore);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the issue where some apps had a white background instead of light gray in the presubmits.

@Splaktar Splaktar added pr: presubmit-failures and removed pr: merge ready This PR is ready for a caretaker to review labels Feb 1, 2019
@Splaktar
Copy link
Contributor Author

Splaktar commented Feb 1, 2019

It looks like the module builds of the CSS (closure or js) are injecting tons of extra theme template styles into component CSS like icon.css and also icon-default-theme.css. Both have increased from ~400 bytes to ~190KB!

This PR tried to ignore the module based builds, but it's clear that I need to go through there and make some tweaks + verify.

@Splaktar Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Feb 1, 2019
@mmalerba mmalerba removed their assignment Feb 6, 2019
@Splaktar Splaktar force-pushed the gulp-deterministicBuilds branch from 944994b to 08a92c0 Compare February 8, 2019 01:47
@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Feb 8, 2019
@Splaktar
Copy link
Contributor Author

Splaktar commented Feb 8, 2019

Duplicate CSS in module-based builds has been resolved. This is ready for presubmit again.

- passing globs to `gulp.src` can cause ordering to not be preserved.
this is because `gulp.src` is async
- gulp-concat tries to preserve the order passed into `gulp.src`.
if the globbing is too vague, then there isn't enough info for ordering
- more statically defining the paths like this isn't ideal
but it is acceptable atm since we aren't adding new components
- fix docs menu unwanted left margin
- remove commented out icon styles
- add JSDoc/Closure types
- remove unneeded parameter passed to util.buildModule()
- remove unneeded parameter passed to util.buildJs()

Fixes #11502
@Splaktar Splaktar force-pushed the gulp-deterministicBuilds branch from 08a92c0 to e4bff89 Compare February 11, 2019 20:22
@Splaktar Splaktar modified the milestones: 1.1.13, 1.1.14 Feb 13, 2019
@mmalerba mmalerba merged commit d56d0e7 into master Mar 13, 2019
@Splaktar Splaktar deleted the gulp-deterministicBuilds branch March 13, 2019 20:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P3: important Important issues that really should be fixed when possible. pr: merge ready This PR is ready for a caretaker to review type: build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build: make build output deterministic
7 participants