-
Notifications
You must be signed in to change notification settings - Fork 27.4k
Conversation
…nkwrap.clean.json Previously we would clean up npm-shrinkwarp.json file in order to achieve serialization stability, which would then allow us to create human readable diffs that allow code reviews of npm-shrinkwrap to be meaningful. This cleanup process does have an impact on the functionality of npm which was only recently discovered by Vojta, when we tried to update to new Karma version. See: socketio/engine.io-client#370 According to Julie, the root cause of these issues is npm/npm/angular#3581. The workaround implemented in this commit is not to interfere with npm-shrinkwrap.json file, but instead preserve the cleaned up version of its content in npm-shrinkwrap.clean.json which can then be used to produce human readable diffs for code reviews of npm dependency updates.
Thanks! It looks like the intermittent 'cb() never called' issue is coming up. Issue with possible fixes - maybe updating to npm 2.xx would help? npm/npm#5920 |
yeah. I think we should start requiring npm 2.x for angular builds. you said that the issue is coming up but I don't see it and CI is green. where is it coming up? on your local machine? |
The error is on an allowed failure, browserstack-unit. |
oh. I didn't bother to look in there. I thought it was a flake :) |
I'm trying out updating to npm 2.x right now: #11111 |
For the record - the reason for this change is that |
5f70851
to
62a74aa
Compare
ok. I fixed the dependency on node and npm versions so that we always use npm 2.5. let's see if that fixes the one odd thing is that without me messing with logging levels, there is a lot of debugging messages in the logs. see: https://travis-ci.org/angular/angular.js/jobs/51477379 any idea why? |
I doubt that I'll have time to figure that out tomorrow, so if anyone wants to take over this PR, please be my guest. |
My guess - it looks like the npm-shrinkwrap file has a non-standard version of Karma:
That probably has some extra logging enabled. Whereas the previous one just had karma@0.12.23. This is because the version of karma in package.json is |
FYI : d64bf98 |
but that's the version we've been using in the last month and the logging was not there, so I don't know why is it there now. |
oh. I see. I accidentally updated karma from
to
I think someone (vojta?) modified the package.json but didn't update the shrinkwrap before, the change took effect only now. I'll go back and change that to the latest karma. |
ok. I believe I managed to untangle the mess and we should be all good. If CI passes, we should merge this in and then hurry up and upgrade karma to use socket.io 1.x and then upgrade karma in angular again. |
hmm... the ci still prints debug statements. anyone knows why?
notice the "got results 50" string source: https://travis-ci.org/angular/angular.js/jobs/51548811 |
I can't repro this locally even when I use the dots reporter like travis. I wonder if the travis cache is serving up the old node_modules version and npm is silly enough to not upgrade the node_modules now that node_modules directory doesn't match the requirements in npm-shrinkwrap.json. I'll try to blow away the node_modules cache and see if that helps |
yes, as suspected. that was the problem! see: https://travis-ci.org/angular/angular.js/jobs/51550774 so @tbosch came up with a nice trick to work around this (though this must be fixed in npm). The trick is as to modify the
This way we retain the benefits of the cache but also our sanity when shrinkwrap changes. It would be even better to make this part of the grunt init so that we do the same in non-ci environment. |
@mzgol I'll try that. thanks! |
8cf5b32
to
9b26f17
Compare
… changes `npm install` blindly accepts the node_modules cache and doesn't verify if it matches requirements in the current npm-shrinkwrap.json. This means that if we are using travis cache and npm-shrinkwrap.json changes npm will keep on using the old dependencies, in spite of the guarantees that shrinkwrap claims to offer. angular#11110 (comment)
9b26f17
to
38fd5ea
Compare
… changes `npm install` blindly accepts the node_modules cache and doesn't verify if it matches requirements in the current npm-shrinkwrap.json. This means that if we are using travis cache and npm-shrinkwrap.json changes npm will keep on using the old dependencies, in spite of the guarantees that shrinkwrap claims to offer. angular#11110 (comment)
50c0d50
to
3fa1a4f
Compare
@mzgol if I switch to node 0.12, karma 0.12.31 fails to install fsevents on mac:
|
50a918b
to
6734341
Compare
… changes `npm install` blindly accepts the node_modules cache and doesn't verify if it matches requirements in the current npm-shrinkwrap.json. This means that if we are using travis cache and npm-shrinkwrap.json changes npm will keep on using the old dependencies, in spite of the guarantees that shrinkwrap claims to offer. angular#11110 (comment) With this change, we will blow away the node_modules directory if the shrinkwrap changes compared to the one used to populate node_modules.
b922f02
to
2339de3
Compare
Alright, I believe that this PR is good to be merged now. The CI is green https://travis-ci.org/angular/angular.js/builds/51605768 and all known issues are resolved except for upgrade to node 0.12, which is out of scope. Can someone take one more last look please? |
Everything looks good to me.
|
@IgorMinar The failures are from dgeni-packages, not Karma (they're still soft failures only btw, it's usable). The reason is the same - dgeni-packages should get EDIT: Actually,
The latest nunjucks installs correctly on Node 0.12. |
@IgorMinar I reported angular/dgeni-packages#108 for the dgeni-packages Node 0.12 issue. |
currently karma's dependencies don't install on node 0.12 or io.js so we'll just update npm in hope that that this will mitigate "cb() never called!" erorrs. See: https://travis-ci.org/angular/angular.js/jobs/51474043#L2181
previously we thought that git:// was enough, but we also want git+https:// otherwise we can miss important info in clean shrinkwrap file
it usually contains urls to temp directories which are not interesting, the info we do want to preserve is in the `resolved` property and we do keep that one.
Previously Vojta set us up to use a custom fork of Karma that used socket.io 1.3.4. This change moves us to an official release of Karma but downgrades socket.io back to 0.9.16. We now need to hurry up and finish the socket.io upgrade in karma which was blocked on shrinkwrap issues in Angular that are resolved with the previous few commits in this PR.
… changes `npm install` blindly accepts the node_modules cache and doesn't verify if it matches requirements in the current npm-shrinkwrap.json. This means that if we are using travis cache and npm-shrinkwrap.json changes npm will keep on using the old dependencies, in spite of the guarantees that shrinkwrap claims to offer. angular#11110 (comment) With this change, we will blow away the node_modules directory if the shrinkwrap changes compared to the one used to populate node_modules.
`du` returns error code 2 when any of the directories don't exist which breaks the build. this scenario is common when the cache was emptied or when travis is building forks that don't have travis cache enabled
…changes this replicates the travis setup in grunt from the previous commit the reason why we duplicate this rather than having just a single place for this code is so that we can individually time the actions on travis
…into install-dependencies.sh So now we are DRY. Added extra error checking and improved the grunt file init setup so that stdio is visible in console.
aha! thanks @mzgol I'll still postpone node update until the issue is fixed in dgeni. I don't want errors in logs even if they are harmless. |
2339de3
to
4286557
Compare
ok. I made a few more changes. the last two commits add the same safecheck to grunt, so that even in dev mode we are safe. and then I extracted the logic into a common script. if CI passes I'm going to merge this into master |
ef4e28e
to
4286557
Compare
… changes `npm install` blindly accepts the node_modules cache and doesn't verify if it matches requirements in the current npm-shrinkwrap.json. This means that if we are using travis cache and npm-shrinkwrap.json changes npm will keep on using the old dependencies, in spite of the guarantees that shrinkwrap claims to offer. #11110 (comment) With this change, we will blow away the node_modules directory if the shrinkwrap changes compared to the one used to populate node_modules.
… changes `npm install` blindly accepts the node_modules cache and doesn't verify if it matches requirements in the current npm-shrinkwrap.json. This means that if we are using travis cache and npm-shrinkwrap.json changes npm will keep on using the old dependencies, in spite of the guarantees that shrinkwrap claims to offer. angular#11110 (comment) With this change, we will blow away the node_modules directory if the shrinkwrap changes compared to the one used to populate node_modules.
…into install-dependencies.sh So now we are DRY. Added extra error checking and improved the grunt file init setup so that stdio is visible in console. Closes angular#11110
… changes `npm install` blindly accepts the node_modules cache and doesn't verify if it matches requirements in the current npm-shrinkwrap.json. This means that if we are using travis cache and npm-shrinkwrap.json changes npm will keep on using the old dependencies, in spite of the guarantees that shrinkwrap claims to offer. angular#11110 (comment) With this change, we will blow away the node_modules directory if the shrinkwrap changes compared to the one used to populate node_modules.
…into install-dependencies.sh So now we are DRY. Added extra error checking and improved the grunt file init setup so that stdio is visible in console. Closes angular#11110
cc: @juliemr