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

remove npm upgrade in .travis.yml #1869

Closed
boneskull opened this issue Sep 7, 2015 · 14 comments
Closed

remove npm upgrade in .travis.yml #1869

boneskull opened this issue Sep 7, 2015 · 14 comments
Assignees

Comments

@boneskull
Copy link
Contributor

currently we run scripts/ensure-compatible.npm.sh to upgrade npm if it's old. since we want to retain support for old npm versions (for now), let's not do that.

@boneskull boneskull self-assigned this Sep 7, 2015
@boneskull
Copy link
Contributor Author

okay, this is not possible due to the coffee-script dev dep.

@boneskull
Copy link
Contributor Author

so, theoretically, we are fine with npm pre-1.3.7 in production--i.e., npm install mocha

but npm pre-1.3.7 is not supported for development:

$ git clone https://github.com/mochajs/mocha.git; cd mocha; npm install

@boneskull
Copy link
Contributor Author

cc @dougwilson

@dougwilson
Copy link
Contributor

So, is coffeescript is only needed for development, or is it also needed to run the tests on Travis CI? If it is not needed to run tests, then you can always remove it before Travis CI does the install (similar to https://github.com/jshttp/mime-db/blob/master/.travis.yml#L13-L21).

@dougwilson
Copy link
Contributor

And of course, if you actually do need coffeescript for running tests, you can always split tests in two parts (kinda like https://github.com/dougwilson/nodejs-depd/blob/master/.travis.yml#L13-L15 + https://github.com/dougwilson/nodejs-depd/blob/master/test/browserify.js#L9).

@dougwilson
Copy link
Contributor

Looks like it is at least used in your test-compilers Makefile target https://github.com/mochajs/mocha/blob/master/Makefile#L55 We could probably jiggle things around to get it to work. If you are serious about removing the npm upgrade, let me know, and I can even perhaps work on a PR to do so instead of you feeling like the burden is on you :)

@boneskull
Copy link
Contributor Author

Is it a big deal though? Do you need to develop Mocha on a system with an old npm version?

@boneskull
Copy link
Contributor Author

I have pushed code for 2.3.2 but haven't published yet

@dougwilson
Copy link
Contributor

Is it a big deal though? Do you need to develop Mocha on a system with an old npm version?

I have to be able to install mocha to test on the systems that I support. I'm not developing on those systems, but mocha is the test runner, so without being able to install mocha, I cannot run any tests against the systems that will be running production code, which would make using mocha not very useful to me.

@boneskull
Copy link
Contributor Author

Well, you can still do that (in theory), you just can't git clone mocha and install its devdependencies. Am I missing something?

@dougwilson
Copy link
Contributor

No, you're not missing anything. The main problem right now is that if you cannot do the install on Travis with an old version of npm, you'll never notice when a change breaks the install process for production users.

@dougwilson
Copy link
Contributor

In other words, it's generally in your favor for your CI process to prevent the need to make commits like c7a8fed , but the current CI process in this repository does not assist in identifying breaking changes.

@boneskull
Copy link
Contributor Author

Right.

I'd consider a solution similar to what you posted above (https://github.com/dougwilson/nodejs-depd/blob/master/.travis.yml#L13-L15). This is fine for CI, but if a user is running npm test locally, they will run into problems. Two more ideas:

  1. Have a prepublish script which installs coffee-script if the detected npm version is high enough (and if there's a env var that npm sets if it's installing in development mode? failing that, presence of .git/ may be sufficient), and then have a conditional which skips the test if coffee-script is not present.
  2. Tell Travis to upgrade npm (if necessary), install coffee-script, then downgrade it again, then npm install. Because we know coffee-script would break an old npm, this is fine. If any other dependency would break stuff, then the build would error.

I'm partial to 2 since it avoids modifying the dependencies or tests themselves.

@dougwilson
Copy link
Contributor

This is fine for CI, but if a user is running npm test locally, they will run into problems.

What problem will they run into? If you clone that repo and run the tests, you'll see there is no issue running them at all (the suggestion consisted of two different areas).

and then have a conditional which skips the test if coffee-script is not present.

That's my original suggestion, the one you referenced with the link to the .travis.yml file :)

Tell Travis to upgrade npm (if necessary), install coffee-script, then downgrade it again, then npm install. Because we know coffee-script would break an old npm, this is fine. If any other dependency would break stuff, then the build would error.

I tried this in the past with my modules but was never able to get the downgrade to work. This means that I would not be successful in implementing this for you, so the implementation would have to be left on your shoulders :(

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

No branches or pull requests

2 participants