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

sim deployment instructions are broken. #39

Closed
pixelzoom opened this issue Jan 6, 2017 · 29 comments
Closed

sim deployment instructions are broken. #39

pixelzoom opened this issue Jan 6, 2017 · 29 comments
Assignees
Labels

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 6, 2017

After updating requirejs (phetsims/chipper#538) the sim deployment instructions are broken (as I discovered in phetsims/ph-scale#69 (comment)). There is apparently one or more "npm update" steps that now need to be manually done (when switching branches?)

All of the instructions in https://github.com/phetsims/phet-info/tree/master/deployment-info should be revised and tested.

High priority because we can't currently deploy maintenance releases and phetsims/ph-scale#69 is blocked.

@jonathanolson
Copy link
Contributor

A summary of what the automated maintenance releases process does:

this.gitCheckoutShas( simName, branch, function() {
  // bump version in package.json
  self.execute( 'git', [ 'add', 'package.json' ], '../' + simName, function() {
    self.execute( 'git', [ 'commit', '-m', 'Bumping version to ' + newVersionString + ' for ' + message ], '../' + simName, function() {
      self.gitPush( simName, branch, function() {
        self.npmUpdate( simName, function() {
          self.npmUpdate( 'chipper', function() {
            self.execute( GRUNT_CMD, [], '../' + simName, function() {
              self.execute( GRUNT_CMD, [ 'deploy-rc' ], '../' + simName, function() {
                self.gitCheckoutMaster( simName, function() {
                  self.success( 'Deployed ' + simName + ' ' + newVersionString );

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 6, 2017

Here's what I'm seeing when trying to build from the 1.2 branch of ph-scale:

% cd ph-scale
% git checkout 1.2
% grunt checkout-shas
% grunt
>> Local Npm module "grunt-requirejs" not found. Is it installed?
>> Local Npm module "grunt-requirejs" not found. Is it installed?
Warning: Task "requirejs:build" not found. Use --force to continue.

Aborted due to warnings.
%

@samreid
Copy link
Member

samreid commented Jan 6, 2017

Instructions were updated in the preceding commit. I also tested that I received the same errors as @pixelzoom before npm update and that after I ran npm update then build completes successfully.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 6, 2017

I realize that the instructions say nothing about how to get back to master following a release. But post-release, do we now need to run 3 commands? i.e.:

% grunt checkout-master
% git checkout master
% npm update

@samreid
Copy link
Member

samreid commented Jan 6, 2017

Yes, and I believe you also need to run npm update in chipper as well. I'll update the docs.

@pixelzoom
Copy link
Contributor Author

Make that 5 manual commands:

% grunt checkout-master
% git checkout master
% npm update
% cd ../chipper
% npm update

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 6, 2017

The instructions now say:

  • Run npm update in the sim repo and in chipper

... but that's really only necessary if you're on a non-master branch, non?

@samreid
Copy link
Member

samreid commented Jan 6, 2017

... but that's really only necessary if you're on a non-master branch, non?

The RC and Production instructions assume that you are working from a set of SHAs, not from master.

@pixelzoom
Copy link
Contributor Author

Ah, right.

@samreid
Copy link
Member

samreid commented Jan 6, 2017

I added the steps to restore working copy above. @pixelzoom anything else to do for this issue?

@samreid samreid removed their assignment Jan 6, 2017
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 8, 2017

The 5 lines added below are now duplicated in 8 files. Doesn't this defeat the reason for the current "build instructions" system? We haven't decided to retire that system (#40 is still open), so it seems like these "restore" instructions should appear once in a new file, and be added to the 8 docs as part of building the instructions.

 +Restore your working copy (optional)
 +- [ ] Check out master for dependencies: `grunt checkout-master`
 +- [ ] Check out master for the sim repo: `git checkout master`
 +- [ ] Update node_modules for the sim: `npm update`
 +- [ ] Navigate to chipper: `cd ../chipper`
 +- [ ] Update node_modules for chipper: `npm update`

@pixelzoom pixelzoom assigned samreid and unassigned pixelzoom Jan 8, 2017
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 10, 2017

Attempting to deploy ph-scale from 1.2 branch with revised instructions. Are these 2 steps in the wrong order?

  • Check out the correct SHAs using grunt checkout-shas.
  • Run npm update in the sim repo and in chipper

I see:

% grunt checkout-shas
>> Local Npm module "grunt-requirejs" not found. Is it installed?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 10, 2017

The steps should probably be:

  • Run npm update in the sim repo.
  • Run grunt checkout-shas in the sim repo.
  • Run npm update in chipper repo

@samreid
Copy link
Member

samreid commented Jan 10, 2017

That should work, but should only be necessary if npm update commands in the "restore" instructions were forgotten: #39 (comment)

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 10, 2017

I don't think that's correct. The sim branch (e.g. pheh-scale 1.2) has requirejs in its package.json. The master branch does not. So when I checkout a branch and run grunt, it complains.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 10, 2017

The "restore" instructions also seem to have problems. Here's what I see for ph-scale, note the multiple npm errors:

% grunt checkout-shas
% git checkout master
% npm update
npm ERR! Darwin 15.6.0
npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "update"
npm ERR! node v4.0.0
npm ERR! npm  v2.14.2
npm ERR! code EPEERINVALID

npm ERR! peerinvalid The package grunt@1.0.1 does not satisfy its siblings' peerDependencies requirements!
npm ERR! peerinvalid Peer grunt-requirejs@0.4.2 wants grunt@~0.4.0
npm ERR! peerinvalid Peer grunt-eslint@19.0.0 wants grunt@>=0.4.0

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/cmalley/PhET/GitHub/ph-scale/npm-debug.log
%

In #39 (comment), I said:

All of the instructions in https://github.com/phetsims/phet-info/tree/master/deployment-info should be revised and tested.

@samreid It sure looks like you skipped the "test" step.

@pixelzoom
Copy link
Contributor Author

Perhaps the "restore" instructions should involve rm -rf node_modules and npm install, instead of npm update.

@jonathanolson
Copy link
Contributor

This is potentially a sign of a greater problem. I've been under the assumption that you can change the package.json dependencies arbitrarily and still have 'npm update' successfully install the needed dependencies.

@samreid
Copy link
Member

samreid commented Jan 11, 2017

I have a vague recollection about npm prune--perhaps it is necessary in some cases? I think the build server code would be the place to look, we tested a variety of options and implemented the best one in the build server.

@samreid
Copy link
Member

samreid commented Jan 11, 2017

Indeed perennial shows how the updates must be done:

                    exec( 'git checkout ' + repos[ simName ].sha, simDir, function() { // checkout the sha for the current sim
                      exec( 'npm prune', '../chipper', function() {
                        exec( 'npm update', '../chipper', function() { // npm update in chipper in case there are new dependencies there
                          exec( 'npm prune', simDir, function() {
                            exec( 'npm update', simDir, function() {

I have no doubt that rm -rf node_modules then npm install would also work, but prune then update was tested across a variety of version changes and appeared to work properly.

@pixelzoom
Copy link
Contributor Author

Since we decided to proceed with #40, this issue is moot.

@samreid
Copy link
Member

samreid commented Jan 13, 2017

phetsims/chipper#542 may also help.

@zepumph
Copy link
Member

zepumph commented Jan 18, 2017

@samreid and @pixelzoom it sounds like all issues revolving around npm prune are created and being taken care of. Since phetsims/chipper#542 is solving this for the main deployment, I think we are clear to transfer these lines: #39 (comment) to the combined and new sim deployment docs in #40. I think we are ready to close this.

@samreid
Copy link
Member

samreid commented Jan 24, 2017

Until phetsims/chipper#542 is complete, we should add npm prune before each npm update command in the instructions. @zepumph can you help with this?

@samreid samreid assigned zepumph and unassigned samreid Jan 24, 2017
@zepumph
Copy link
Member

zepumph commented Jan 24, 2017

Yes, how about I just work on phetsims/chipper#542 instead of the temporary fix.

@samreid
Copy link
Member

samreid commented Jan 24, 2017

phetsims/chipper#542 will only help on master sims released after phetsims/chipper#542 is published. Sims using older shas will need the manual instructions.

zepumph added a commit that referenced this issue Jan 25, 2017
zepumph added a commit that referenced this issue Jan 25, 2017
@zepumph
Copy link
Member

zepumph commented Jan 25, 2017

Ah yes that makes sense,

Step 8. Restore your working copy

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

This is what it looks like now. I am tempted to change it to:

Step 8. Restore your working copy

  • Checkout chipper master: cd ../chipper; git checkout master
  • CD back to sim repo
  • Check out master for dependencies: grunt checkout-master

It would always use master's grunt checkout-master so it will always prune and update. Perhaps that would be easier? Not sure what people will think.

@samreid
Copy link
Member

samreid commented Jan 25, 2017

I think in phetsims/chipper#542 you recommended running grunt checkout-master twice, that sounded reasonable to me. Would it solve this problem?

zepumph added a commit that referenced this issue Jan 25, 2017
@zepumph
Copy link
Member

zepumph commented Jan 25, 2017

Yeah I actually like that most. Closing.

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

No branches or pull requests

4 participants