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 use of npm ls in grunt tasks #11965

Merged
merged 8 commits into from
May 24, 2017

Conversation

spalger
Copy link
Contributor

@spalger spalger commented May 22, 2017

In order to transition to yarn we have to remove our dependency on npm ls, which is currently being used in the grunt tasks to get the paths for modules. To remove this dependency, this pr passes the customFormat argument to license-checker, which adds the directory for each module to its output that already included every installed npm package.

The two tasks using npm ls are licenses and _build:notice. Now both of those tasks use the getInstalledPackages() function from tasks/lib to fetch a list of packages (created with the license-checker module then normalized) and then pass the list of packages to either assertValidLicenses() or generateNoticeText(), also from tasks/lib.

Care was taken to not bind the logic to grunt directly so that we can move away from grunt for these tasks someday.

The _build:notice task used to rely on the output of `npm ls` to determine where modules were defined, but the task now just asks `license-checker` to include the `realPath` of the modules it describes in it's output, which is ultimately the same thing but works with `yarn` too.
@jbudz
Copy link
Member

jbudz commented May 23, 2017

Is this expected?

$ grunt licenses
Running "licenses" task
Fatal error: Path must be a string. Received true

@jbudz
Copy link
Member

jbudz commented May 23, 2017

Can we remove npm from dev dependencies?

@spalger
Copy link
Contributor Author

spalger commented May 23, 2017

Is this expected?

$ grunt licenses
Running "licenses" task
Fatal error: Path must be a string. Received true

No, and I'm not seeing that locally, can you run and pass --verbose --stack to grunt so that we get more info?

@jbudz
Copy link
Member

jbudz commented May 23, 2017

I cleared out node modules and it's fine now, sorry about that. Maybe I had some local changes.

Running "licenses"
Options: licenses=["(BSD-2-Clause OR MIT OR Apache-2.0)","(BSD-2-Clause OR MIT)","(MIT AND CC-BY-3.0)","(MIT OR Apache-2.0)","AFLv2.1","Apache","Apache 2.0","Apache License, v2.0","Apache*","Apache, Version 2.0","Apache-2.0","BSD","BSD New","BSD*","BSD-2-Clause","BSD-3-Clause","BSD-3-Clause AND MIT","BSD-3-Clause OR MIT","BSD-like","CC-BY","CC-BY-4.0","ISC","MIT","MIT*","MIT/X11","OFL-1.1 AND MIT","Public domain","Unlicense","WTFPL","WTFPL OR ISC","new BSD, and MIT"], overrides={"assert-plus@0.1.5":["MIT"],"buffers@0.1.1":["MIT/X11"],"bytes@1.0.0":["MIT"],"color-name@1.0.0":["UNLICENSE"],"commander@2.2.0":["MIT"],"css-color-names@0.0.1":["MIT"],"css-parse@1.0.4":["MIT"],"css-stringify@1.0.5":["MIT"],"css@1.0.8":["MIT"],"delegate@3.0.1":["MIT"],"flatten@0.0.1":["MIT"],"indexof@0.0.1":["MIT"],"ripemd160@0.2.0":["MIT"],"select@1.0.6":["MIT"],"uglify-js@2.2.5":["BSD"],"ua-parser-js@0.7.12":["MIT"]}                                      Fatal error: Path must be a string. Received
TypeError: Path must be a string. Received true
  at assertPath (path.js:7:11)
  at relative (path.js:1227:5)
  at /Users/jbudz/Projects/elastic/kibana/tasks/lib/packages/installed_packages.js:51:19
  at Array.map (native)
  at /Users/jbudz/Projects/elastic/kibana/tasks/lib/packages/installed_packages.js:31:6
  at next (native)
  at step (/Users/jbudz/Projects/elastic/kibana/tasks/lib/packages/installed_packages.js:64:191)                                         
  at /Users/jbudz/Projects/elastic/kibana/tasks/lib/packages/installed_packages.js:64:361                                                
  at process._tickDomainCallback (internal/process/next_tick.js:135:7)

@tylersmalley tylersmalley self-requested a review May 23, 2017 19:01
Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

Looking good. I tested on both windows and linux, diffed the notice files, and broke a few licenses.

Everything looked fine except the node.js license and base_notice.txt are copied over twice to the final notice file.

@jbudz
Copy link
Member

jbudz commented May 23, 2017

Also noting I ran into the same path must be a string issue on windows too, until I did another npm install. My deps were a few weeks out of date.

@tylersmalley
Copy link
Contributor

So far this is looking good to me as well. When diffing the two NOTICE.txt files, I see we're now including duplicates, which is evident of being almost twice as large (15207 lines vs 30353). Is this something you feel we should resolve, or is the level of effort involved not worth it so long as the NOTICE file includes all shipped dependencies?

@spalger
Copy link
Contributor Author

spalger commented May 23, 2017

@jbudz @tylersmalley Sorry, I should have known this was coming.

The issue is that getInstalledPackages() included the root package (kibana in this case) and when it builds the NOTICE.txt file it looks for notice/license files from every installed package which means that when running _build:notice multiple times it will include the previous NOTICE.txt in the new one.

Fixed this in e5a4839

@spalger
Copy link
Contributor Author

spalger commented May 23, 2017

Here is my diff after running grunt build in both master and then in this pr: https://gist.github.com/spalger/89f6c062dcf86395e944871f08d67344

Everything looked fine except the node.js license and base_notice.txt are copied over twice to the final notice file.

Looks like this is the behavior in master too, but e5a4839 removes it and I'm confident that's fine/good.

@tylersmalley
Copy link
Contributor

@spalger I am seeing the same diff and agree we should remove the kibana package bundle.

LGTM

@spalger spalger merged commit 5c04ff6 into elastic:master May 24, 2017
spalger added a commit to spalger/kibana that referenced this pull request May 24, 2017
* [grunt/build] refactor _build:notice task to not depend on npm

The _build:notice task used to rely on the output of `npm ls` to determine where modules were defined, but the task now just asks `license-checker` to include the `realPath` of the modules it describes in it's output, which is ultimately the same thing but works with `yarn` too.

* [grunt/licenses] convert to use lib/packages/getInstalledPackages()

* [grunt/notice/generate] test generateNoticeText()

* [grunt/licenses] tested assertLicensesValid()

* [npm] remove npm dev dep

* [tasks/lib/packages] do not include kibana in "installed packages"

* [tasks/lib/notice] join all notices with the same separator

(cherry picked from commit 5c04ff6)
spalger added a commit to spalger/kibana that referenced this pull request May 24, 2017
* [grunt/build] refactor _build:notice task to not depend on npm

The _build:notice task used to rely on the output of `npm ls` to determine where modules were defined, but the task now just asks `license-checker` to include the `realPath` of the modules it describes in it's output, which is ultimately the same thing but works with `yarn` too.

* [grunt/licenses] convert to use lib/packages/getInstalledPackages()

* [grunt/notice/generate] test generateNoticeText()

* [grunt/licenses] tested assertLicensesValid()

* [npm] remove npm dev dep

* [tasks/lib/packages] do not include kibana in "installed packages"

* [tasks/lib/notice] join all notices with the same separator

(cherry picked from commit 5c04ff6)
spalger added a commit that referenced this pull request May 24, 2017
* [grunt/build] refactor _build:notice task to not depend on npm

The _build:notice task used to rely on the output of `npm ls` to determine where modules were defined, but the task now just asks `license-checker` to include the `realPath` of the modules it describes in it's output, which is ultimately the same thing but works with `yarn` too.

* [grunt/licenses] convert to use lib/packages/getInstalledPackages()

* [grunt/notice/generate] test generateNoticeText()

* [grunt/licenses] tested assertLicensesValid()

* [npm] remove npm dev dep

* [tasks/lib/packages] do not include kibana in "installed packages"

* [tasks/lib/notice] join all notices with the same separator

(cherry picked from commit 5c04ff6)
spalger added a commit that referenced this pull request May 24, 2017
* [grunt/build] refactor _build:notice task to not depend on npm

The _build:notice task used to rely on the output of `npm ls` to determine where modules were defined, but the task now just asks `license-checker` to include the `realPath` of the modules it describes in it's output, which is ultimately the same thing but works with `yarn` too.

* [grunt/licenses] convert to use lib/packages/getInstalledPackages()

* [grunt/notice/generate] test generateNoticeText()

* [grunt/licenses] tested assertLicensesValid()

* [npm] remove npm dev dep

* [tasks/lib/packages] do not include kibana in "installed packages"

* [tasks/lib/notice] join all notices with the same separator

(cherry picked from commit 5c04ff6)
spalger added a commit that referenced this pull request May 24, 2017
* [grunt/build] refactor _build:notice task to not depend on npm

The _build:notice task used to rely on the output of `npm ls` to determine where modules were defined, but the task now just asks `license-checker` to include the `realPath` of the modules it describes in it's output, which is ultimately the same thing but works with `yarn` too.

* [grunt/licenses] convert to use lib/packages/getInstalledPackages()

* [grunt/notice/generate] test generateNoticeText()

* [grunt/licenses] tested assertLicensesValid()

* [npm] remove npm dev dep

* [tasks/lib/packages] do not include kibana in "installed packages"

* [tasks/lib/notice] join all notices with the same separator

(cherry picked from commit 5c04ff6)
@spalger
Copy link
Contributor Author

spalger commented May 24, 2017

5.5/5.x: 098242d
5.4: 5586557
5.3: 9ed5ca6

@spalger spalger deleted the refactor/grunt/no-npm-ls branch May 24, 2017 16:52
snide pushed a commit to snide/kibana that referenced this pull request May 30, 2017
* [grunt/build] refactor _build:notice task to not depend on npm

The _build:notice task used to rely on the output of `npm ls` to determine where modules were defined, but the task now just asks `license-checker` to include the `realPath` of the modules it describes in it's output, which is ultimately the same thing but works with `yarn` too.

* [grunt/licenses] convert to use lib/packages/getInstalledPackages()

* [grunt/notice/generate] test generateNoticeText()

* [grunt/licenses] tested assertLicensesValid()

* [npm] remove npm dev dep

* [tasks/lib/packages] do not include kibana in "installed packages"

* [tasks/lib/notice] join all notices with the same separator
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.

None yet

3 participants