"Restore your working copy" process is broken

[pixelzoom]

Deploying a PhET Simulation says:

Step 8. Restore your working copy

• Check out master for dependencies: grunt checkout-master
• Run again to prune and update node modules: grunt checkout-master, Skip This Step if you are using a chipper sha that is newer than Jan 24th, 2017.
• Check out master for the sim repo: git checkout master

Following those instructions fails. Here's an example after doing a maintenance release of function-builder 1.0:

% cd function-builder
% git checkout 1.0
% grunt checkout-shas
// do a maintenance release here
% grunt checkout-master
% grunt checkout-master 
Loading "Gruntfile.js" tasks...ERROR
>> Error: Cannot find module 'istanbul'
Warning: Task "checkout-master" not found. Use --force to continue.

Aborted due to warnings.
%

[pixelzoom]

Here's a short-term workaround, which requires manually fixing chipper before doing the second checkout-master.

% cd function-builder
% git checkout 1.0
% grunt checkout-shas
// do a maintenance release here
% grunt checkout-master
% cd ../chipper
% npm prune
% npm update
% cd ../function-builder
% grunt checkout-master 
% git checkout master

[samreid]

What if we bail on npm and just check everything in to sherpa? (We would still need to npm update after checkout-shas to support legacy versions, but going forward this process would be simpler.).

[samreid]

Or should we just paste the instructions with npm prune and update https://github.com/phetsims/phet-info/issues/44#issuecomment-296453008 into the instructions and call it good?

[pixelzoom]

First, I'd like to know why istanbul (code coverage tool) is necessary for the checkout-master task -- that makes no sense to me. Who added this dependency? What is it actually being used for?...

@samreid asked:

https://github.com/phetsims/phet-info/issues/44#issuecomment-296705619

Ummm... No. Having to run checkout-master twice is already enough brain damage.

[samreid]

Ummm... No. Having to run checkout-master twice is already enough brain damage.

For future versions (not legacy), then you would have to run checkout-master once. What is the argument against bailing on npm?

[samreid]

Who added this dependency? What is it actually being used for?

@jonathanolson added istanbul as a code coverage tool which runs during the build step.

[pixelzoom]

What is the argument against bailing on npm?

I'm not clear on what you're suggesting. Are you suggesting that we manage (in sherpa) everything that npm currently installs? That sounds like a lot of work. And we have enough trouble staying up-to-date with the relatively small number of 3rd-party libs that we currently use.

@jonathanolson added istanbul as a code coverage tool which runs during the build step

Presumably that means during the build task. Why is it required for checkout-master?

[pixelzoom]

@jonathanolson added istanbul as a code coverage tool which runs during the build step

@jonathanolson: (1) How is istanbul adding value to the build task? (2) Why is istanbul required for the checkout-master task?

[samreid]

Are you suggesting that we manage (in sherpa) everything that npm currently installs? That sounds like a lot of work.

Yes, that was the main idea. Grunt may make that impossible though, depending on how it works. (may require local node_modules).

Presumably that means during the build task. Why is it required for checkout-master?

It could be that it gets resolved during module-load time and if any module fails to load, then the process fails.

[jonathanolson]

How is istanbul adding value to the build task?

It allows checking code coverage for sims, which has been helpful.

Why is istanbul required for the checkout-master task?

The main gruntfile requires generateCoverage, which depends on istanbul. Most Node things assume that the node_modules has things from package.json installed.

Presumably this shouldn't be an issue with "npm prune" and "npm update". For a while I've recommended against using chipper's "checkout-master" since it can have this issue, and instead recommend perennial's "grunt checkout-master --repo=...".

[samreid]

perennial sounds like a great solution to this. @pixelzoom can you test it out and if it works well, update the instructions?

[pixelzoom]

@jonathanolson said:

For a while I've recommended against using chipper's "checkout-master" since it can have this issue, and instead recommend perennial's "grunt checkout-master --repo=...".

@samreid said:

perennial sounds like a great solution to this.

perennial has its own checkout-master task, with different options and a different .js file?!? That's not confusing at all :) Having 2 different tasks with the same name, and no documentation in their .js file to indicate why they are different, does not sound like a "great solution". But I'll take it for a test drive.

[jonathanolson]

perennial has its own checkout-master task, with different options and a different .js file?!? That's not confusing at all :) Having 2 different tasks with the same name, and no documentation in their .js file to indicate why they are different, does not sound like a "great solution".

Agreed, definitely not ideal. I think we should have one solution in perennial for this, since we really should be running the same code both directions (for master=>SHAs and SHAs=>master), instead of running two different chipper versions.

[samreid]

So maybe delete the chipper one and update instructions and processes to rely on the perennial one?

[pixelzoom]

Adding developer-meeting label to discuss, so we can (a) make all developers aware that it's broken, (b) decide how to fix it, and (c) decide who will fix it.

[pixelzoom]

@jonathanolson pointed out that the workaround in https://github.com/phetsims/phet-info/issues/44#issuecomment-296453008 can fail in some circumstances. You really need to npm prune and npm install after checkout-shas.

[pixelzoom]

4/27/17 dev meeting notes:

Sounds like we should ditch the chipper checkout-shas and checkout-master tasks, and use perennial's checkout-shas, with npm install and npm update added (which probably requires changes to builder-server, since it's doing those npm operations elsewhere).

Deferred to 5/4/17 dev meeting, when @mattpen is here, since he's done some working on perennial and build-server.

[Denz1994]

Assigning myself to review this issue's process in an effort to learn more about the build process, as per suggestion by @ariel-phet.

[jonathanolson]

Will plan to move to perennial's version. I'll update documentation and process documents.