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.

Sign up for GitHub

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

Jump to bottom

Fix license check script to ignore sub-dependencies of ignored packages #21606

Merged
merged 5 commits into from
Apr 21, 2020
Merged

Fix license check script to ignore sub-dependencies of ignored packages #21606

merged 5 commits into from
Apr 21, 2020

Conversation

ceyhun
Copy link
Member

@ceyhun ceyhun commented Apr 15, 2020 •
edited
Loading

Description

This is a part of the migration of gutenberg-mobile to the gutenberg repo.
Related to #19861

Updated check-licenses script to parse JSON dependency tree recursively so sub-dependencies of packages passed in --ignore flag are ignored as well.

How has this been tested?

  • npm run check-licenses should complete without errors

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Apr 15, 2020 •
edited
Loading

Size Change: 0 B

Total Size: 842 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.01 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.1 kB 0 B
build/block-library/editor.css 7.1 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/style-rtl.css 7.17 kB 0 B
build/block-library/style.css 7.17 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.54 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 27.9 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.5 kB 0 B
build/edit-site/style-rtl.css 5.04 kB 0 B
build/edit-site/style.css 5.04 kB 0 B
build/edit-widgets/index.js 7.49 kB 0 B
build/edit-widgets/style-rtl.css 4.66 kB 0 B
build/edit-widgets/style.css 4.66 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.3 kB 0 B
build/editor/style-rtl.css 3.48 kB 0 B
build/editor/style.css 3.47 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.32 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@pento pento marked this pull request as ready for review April 15, 2020 23:27
@pento
Copy link
Member

pento commented Apr 16, 2020

This is a bit of a tricky problem, thanks for diving into it, @ceyhun!

To give some history, the check-licenses script was originally written to parse the output of npm ls --parseable, since that was the easiest method to get a list of all dependencies, and we had no need to worry about which module was included by which. There's certainly no need to stick with this model.

This is clearly a special case worth working around, so I would suggest reworking check-licenses to operate on a more structured dependency model.

I suspect the simplest option will be to switch to npm ls --json, and recursing through the dependency tree. That JSON blob already includes path data for each module, which should allow the existing package.json and license file parsing in check-licenses to be reused in a largely unmodified manner.

path = `${ pkg._where }/node_modules${ pkg._location }`;

With this in place, the dependency tree recursor can just skip any module (and its dependencies) that are passed in the --ignore argument. Since @react-native-community/cli has been manually vetted, it can be included in that argument.

@ceyhun ceyhun requested a review from Tug April 16, 2020 13:51
@ceyhun
Copy link
Member Author

ceyhun commented Apr 16, 2020

Thanks for the suggestion @pento! Pushed the changes implementing that and it seems to work, feel free to take a look.

pento
pento approved these changes Apr 17, 2020
View reviewed changes
Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

Looks good to me, nice work on the clean rewrite, @ceyhun!

Given the size of the output from npm ls, there's the potential for weird failures in the future, should the dependency tree grow big enough to overrun the buffer size. I'm okay with leaving it as is for now, we can investigate converting it to using a streaming JSON parser if it turns out to be a problem in the future.

@gziolo
Copy link
Member

gziolo commented Apr 17, 2020 •
edited
Loading

It would be great to cherry-pick changes applied to @wordpress/scripts and merge them to master separately. It looks like a bug in the license check script that might benefit other projects. It would be also nice to include a changelog entry that explains what was fixed:
https://github.com/WordPress/gutenberg/blob/master/packages/scripts/CHANGELOG.md

@Tug
Copy link
Contributor

Tug commented Apr 17, 2020 •
edited
Loading

I agree the script changes should go to master 👍
We can use this PR to target master and revert the other dependency changes, then we'll do another PR that targets the monorepo feature branch and updates master, WDYT @ceyhun ?

@ceyhun
Copy link
Member Author

ceyhun commented Apr 17, 2020

Sounds good 👍

@ceyhun ceyhun changed the base branch from feat/import-gutenberg-mobile-no-squash to master April 17, 2020 09:56
@ceyhun ceyhun changed the title [RNMobile][Monorepo] Fix license check errors Fix license check script to ignore sub-dependencies of ignored packages Apr 17, 2020
@ceyhun ceyhun force-pushed the rnmobile/monorepo-fix-licensing branch from cd0c3cc to b8a745e Compare April 17, 2020 09:59
gziolo
gziolo reviewed Apr 17, 2020
View reviewed changes
@ceyhun ceyhun requested review from Tug and gziolo April 17, 2020 15:41
@gziolo
Copy link
Member

gziolo commented Apr 17, 2020

Awesome, thanks for all updates 👍

Tug
Tug approved these changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@Tug Tug left a comment

Choose a reason for hiding this comment

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

:shipit:

@ceyhun ceyhun merged commit 9227301 into master Apr 21, 2020
@ceyhun ceyhun deleted the rnmobile/monorepo-fix-licensing branch April 21, 2020 12:14
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 21, 2020
@dratwas dratwas mentioned this pull request May 1, 2020
Merged
21 tasks
@Tug Tug mentioned this pull request Jun 5, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tug Tug Tug approved these changes

@pento pento pento approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

@youknowriad youknowriad Awaiting requested review from youknowriad

@gziolo gziolo Awaiting requested review from gziolo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Gutenberg 8.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ceyhun @pento @gziolo @Tug