add yarn.lock file for deterministic builds #7041

erwinmombay · 2017-01-14T19:56:13Z

/to @dknecht as well for review

need to use yarn instead of npm install in deployment systems

erwinmombay · 2017-01-14T19:56:55Z

package.json

@@ -56,7 +56,6 @@
    "gulp-git": "1.7.2",
    "gulp-help": "1.6.1",
    "gulp-if": "2.0.0",
-    "gulp-image-diff": "0.3.0",


had to remove this since it had compatibility issues. i dont think we use it right now anyways

dknecht · 2017-01-14T20:01:29Z

Thanks. I assume this would have prevented #7039

cc / @ipostelnik

aghassemi · 2017-01-15T00:19:35Z

@erwinmombay I assume yarn.lock is not picked up by npm and migrating from npm to yarn requires some work (e.g. updating CONTRIBUTING.md, SERVING.md, travis config and few scripts in package.json that still use npm.

Maybe we can start by a public INTENT TO IMPLEMENT: Switching from npm to yarn issue and take it from there?

erwinmombay · 2017-01-17T17:08:37Z

@aghassemi yeah we will have to update docs for sure. I'll create an issue

aghassemi · 2017-01-17T17:55:34Z

@erwinmombay I am more worried about the work involved in migrating travis config, few scripts in package.json and our internal release scripts to yarn than just the docs. Would be nice to do all that in a single PR after which everyone should switch to yarn.

erwinmombay · 2017-01-17T18:24:12Z

the work on travis is pretty minimal the only tricky part is installing "yarn"

erwinmombay · 2017-01-17T20:58:37Z

.travis.yml

@@ -37,6 +37,7 @@ env:
    - NPM_CONFIG_PROGRESS="false"

 cache:
+  yarn: true


@aghassemi switched to yarn

ampprojectbot · 2017-01-18T09:50:10Z

Here is a list of owners that can approve this PR:

/to @cramforce

Owners can approve through the "APPROVED" label. If the author of the PR is an owner of the files or directory, the PR will be approved right away.

cramforce · 2017-01-18T16:48:00Z

@ampprojectbot You will need a more optimistic profile image for me to approve this change!

aghassemi · 2017-01-18T17:03:23Z

build-system/pr-check.js

@@ -207,6 +207,12 @@ function main(argv) {
    return 0;
  }

+  if (!(files.some(x => x === 'package.json') &&


The logic here fails if PR does not include either of them. We want logical XOR here. Maybe do:

if( files.includes('package.json') ? files.includes('yarn.lock') : !files.includes('yarn.lock') )

good catch. will fix

can you check my logic again. changed it to:

files.includes('package.json') ? !files.includes('yarn.lock') : false

we should be OK if yarn.lock is updated but package.json is not in instances where we re-pin nested deps. unlikely but i see it happen when running yarn update dep

aghassemi · 2017-01-18T17:04:46Z

build-system/pr-check.js

@@ -207,6 +207,12 @@ function main(argv) {
    return 0;
  }

+  if (!(files.some(x => x === 'package.json') &&
+        files.some(x => x === 'yarn.lock'))) {
+      console.error(`\npr-check.js - please update through yarn.\n`);


pr-check.js - any update to package.json or yarn.lock must include the other file. Please update through yarn.

aghassemi · 2017-01-18T17:04:58Z

build-system/tasks/index.js

@@ -24,7 +24,7 @@ require('./csvify-size');
 require('./dep-check');
 require('./get-zindex');
 require('./lint');
-require('./make-golden');
+//require('./make-golden');


remove instead of commenting out?

ampprojectbot · 2017-01-18T17:05:59Z

Here is a list of owners that can approve this PR:

/to @cramforce

Owners can approve through the "APPROVED" label. If the author of the PR is an owner of the files or directory, the PR will be approved right away.

erwinmombay · 2017-01-18T17:29:21Z

bot logic was wrong. fixing here https://github.com/google/github-owners-bot/pull/7/files

aghassemi · 2017-01-18T20:37:40Z

build-system/pr-check.js

@@ -207,6 +207,12 @@ function main(argv) {
    return 0;
  }

+  if (files.includes('package.json') ? !files.includes('yarn.lock') : false) {


I think what I suggested previously was correct:

if has package.json => must also have yarn.lock, otherwise must NOT have yarn.lock

* add yarn.lock file for deterministic builds * switch travis to use yarn * fix logic for package.json an yarn.lock check * make sure that it also errors if only yarn.lock exists * add back yarn.lock file * update serving/building docs

erwinmombay requested review from cramforce and aghassemi January 14, 2017 19:56

erwinmombay commented Jan 14, 2017

View reviewed changes

aghassemi mentioned this pull request Jan 15, 2017

Test failure on Mac due to browserslist not liking op_mini #7031

Closed

erwinmombay force-pushed the add-yarn-lock branch 2 times, most recently from 93cfd99 to b3ceb33 Compare January 17, 2017 20:33

erwinmombay commented Jan 17, 2017

View reviewed changes

erwinmombay force-pushed the add-yarn-lock branch 3 times, most recently from 0fb8c04 to 732e98e Compare January 18, 2017 06:52

erwinmombay added APPROVED and removed APPROVED labels Jan 18, 2017

erwinmombay force-pushed the add-yarn-lock branch 2 times, most recently from d33fb6c to 1a23f67 Compare January 18, 2017 08:53

aghassemi suggested changes Jan 18, 2017

View reviewed changes

erwinmombay added 2 commits January 18, 2017 10:57

add yarn.lock file for deterministic builds

cd68e71

switch travis to use yarn

ff6cc8c

erwinmombay force-pushed the add-yarn-lock branch from 1a23f67 to c1cf5e7 Compare January 18, 2017 18:57

fix logic for package.json an yarn.lock check

f276cf1

erwinmombay force-pushed the add-yarn-lock branch from c1cf5e7 to f276cf1 Compare January 18, 2017 19:09

aghassemi reviewed Jan 18, 2017

View reviewed changes

aghassemi approved these changes Jan 18, 2017

View reviewed changes

erwinmombay added 3 commits January 18, 2017 12:46

make sure that it also errors if only yarn.lock exists

9c84701

add back yarn.lock file

d076565

update serving/building docs

9073448

erwinmombay merged commit e9fd8e2 into ampproject:master Jan 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add yarn.lock file for deterministic builds #7041

add yarn.lock file for deterministic builds #7041

erwinmombay commented Jan 14, 2017 •

edited

erwinmombay Jan 14, 2017

dknecht commented Jan 14, 2017

aghassemi commented Jan 15, 2017

erwinmombay commented Jan 17, 2017

aghassemi commented Jan 17, 2017

erwinmombay commented Jan 17, 2017

erwinmombay Jan 17, 2017

aghassemi Jan 17, 2017

ampprojectbot commented Jan 18, 2017

cramforce commented Jan 18, 2017

aghassemi Jan 18, 2017

erwinmombay Jan 18, 2017

erwinmombay Jan 18, 2017

aghassemi Jan 18, 2017

erwinmombay Jan 18, 2017

aghassemi Jan 18, 2017

erwinmombay Jan 18, 2017

erwinmombay Jan 18, 2017

ampprojectbot commented Jan 18, 2017

erwinmombay commented Jan 18, 2017

aghassemi Jan 18, 2017 •

edited

erwinmombay Jan 18, 2017

erwinmombay Jan 18, 2017

add yarn.lock file for deterministic builds #7041

add yarn.lock file for deterministic builds #7041

Conversation

erwinmombay commented Jan 14, 2017 • edited

Choose a reason for hiding this comment

dknecht commented Jan 14, 2017

aghassemi commented Jan 15, 2017

erwinmombay commented Jan 17, 2017

aghassemi commented Jan 17, 2017

erwinmombay commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ampprojectbot commented Jan 18, 2017

cramforce commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ampprojectbot commented Jan 18, 2017

erwinmombay commented Jan 18, 2017

aghassemi Jan 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay commented Jan 14, 2017 •

edited

aghassemi Jan 18, 2017 •

edited