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

Preinstall check that mentions 'yarn kbn' #16572

Merged
merged 1 commit into from
Feb 8, 2018

Conversation

kimjoar
Copy link
Contributor

@kimjoar kimjoar commented Feb 7, 2018

Some preinstall checks to help people remember yarn kbn bootstrap.

This also throws when using npm. However, it's a bit annoying that npm<5 runs this lifecycle hook after installing deps (and npm 3 doesn't allow our link: deps, so it will likely fail when installing deps). But at least it warns at the right time for npm5+.

Test cases:

  • (with npm3 you'll just get an Unsupported URL Type error, so it's not necessary to test it)
  • with npm5 version (aka node 8), run npm install. It should throw an error about using Yarn instead.
  • run yarn and verify you see a message about preferring to use yarn kbn bootstrap
  • run yarn kbn bootstrap and verify no warnings

Tested on:

  • macos
  • linux
  • windows

@kimjoar kimjoar added the chore label Feb 7, 2018
@@ -0,0 +1,32 @@
const isUsingNpm = process.env.npm_config_git !== undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I searched around, this seemed like the simplest way to know it was npm and not yarn. E.g. from yarn team lead: https://twitter.com/cpojer/statuses/953286334013231104

@epixa epixa added the dev label Feb 7, 2018
@epixa
Copy link
Contributor

epixa commented Feb 7, 2018

Holy hacks. If this were a production thing, I'd say we need integration tests for all of these different possible scenarios, but it's not, and it never will be, so we're probably fine.

@epixa
Copy link
Contributor

epixa commented Feb 7, 2018

It'd be good to test this on macos/linux/windows to ensure our expectations around the process contents are the same across OSs.

@kimjoar
Copy link
Contributor Author

kimjoar commented Feb 7, 2018

I've tested macos.

@azasypkin Can you test this on Linux?
@marius-dr Can you test this on Windows?

(btw, @marius-dr, if you don't have nvm set up to easily switch to Node8, we can get another Windows user to test — I just chose you as I know you're on Windows)

@epixa
Copy link
Contributor

epixa commented Feb 7, 2018

And we should verify that adding/removing dependencies doesn’t trigger a warning. Not sure if that triggers a preinstall

@kimjoar
Copy link
Contributor Author

kimjoar commented Feb 7, 2018

It triggers the preinstall hook, but no warning as argv.cooked.includes('install') returns false.

@nreese
Copy link
Contributor

nreese commented Feb 7, 2018

I think the warnings are very helpful. Thanks @kjbekkelund

@azasypkin
Copy link
Member

OS: Arch Linux

Test case №1:
node v6.12.2 / npm v3.10.10
Unsupported URL Type: link:packages/kbn-babel-preset

node v8.9.4 / npm v5.6.0
throw "Use Yarn instead of npm, see Kibana's contributing guidelines"

Test case №2:
node v6.12.2 / npm v3.10.10
WARNING: When installing dependencies, prefer yarn kbn bootstrap

node v8.9.4 / npm v5.6.0
WARNING: When installing dependencies, prefer yarn kbn bootstrap

The engine "node" is incompatible with this module. Expected version "6.12.2".

Test case №3:
node v6.12.2 / npm v3.10.10
No warnings (except for the incorrect peer dependency old ones)

node v8.9.4 / npm v5.6.0
The engine "node" is incompatible with this module. Expected version "6.12.2".

@kimjoar
Copy link
Contributor Author

kimjoar commented Feb 7, 2018

Thanks for testing. I added a clarification around the npm3 (expected) failure in the description now too.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@marius-dr
Copy link
Member

NPM 3:

npm ERR! node v6.12.2
npm ERR! npm  v3.10.10
**npm ERR! Unsupported URL Type: link:packages/kbn-babel-preset**

NPM 5:


> kibana@7.0.0-alpha1 preinstall C:\Users\ratonbox\Documents\GitHub\kibana
> node ./preinstall_check
C:\Users\ratonbox\Documents\GitHub\kibana\preinstall_check.js:4
  throw "Use Yarn instead of npm, see Kibana's contributing guidelines";
  ^
**Use Yarn instead of npm, see Kibana's contributing guidelines**
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! kibana@7.0.0-alpha1 preinstall: `node ./preinstall_check`
npm ERR! Exit status 1

Yarn:

yarn install v1.3.2
$ node ./preinstall_check

WARNING: When installing dependencies, prefer `yarn kbn bootstrap`

[1/5] Validating package.json...
error kibana@7.0.0-alpha1: The engine "node" is incompatible with this module. E
xpected version "6.12.2".

Yarn kbn bootstrap:

yarn run v1.3.2
$ node scripts/kbn bootstrap
Running [bootstrap] command from [C:\Users\ratonbox\Documents\GitHub\kibana]:

Found [5] projects:  [...]

and it goes on to work.

LGTM

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

Successfully merging this pull request may close these issues.

6 participants