Skip to content

fix(@angular-devkit/build-angular): switch to license-checker-webpack-plugin#16291

Merged
mgechev merged 2 commits intoangular:masterfrom
dgp1130:third-party
Apr 8, 2020
Merged

fix(@angular-devkit/build-angular): switch to license-checker-webpack-plugin#16291
mgechev merged 2 commits intoangular:masterfrom
dgp1130:third-party

Conversation

@dgp1130
Copy link
Copy Markdown
Collaborator

@dgp1130 dgp1130 commented Nov 26, 2019

Fixes #16193, #14095.

This replaces the existing license-webpack-plugin with the new license-checker-webpack-plugin. The former would output multiple 3rdpartylicenses.txt files which would trigger warnings in Webpack. The later does not emit this warning and still appears to log all third party licenses in a reasonable fashion.

The output format is changed, see https://www.diffchecker.com/puCPV7LQ for a more complete before-and-after comparison of a simple example Angular app (with jQuery and Bootstrap installed).

It is worth discussing if we want to go down this route or if we should stick with license-webpack-plugin and attempt to fix it rather than changing the entire dependency.

@dgp1130 dgp1130 added the target: patch This PR is targeted for the next patch release label Nov 26, 2019
@dgp1130 dgp1130 added this to the v9-blockers milestone Nov 26, 2019
@dgp1130 dgp1130 requested a review from clydin November 26, 2019 22:05
@dgp1130 dgp1130 self-assigned this Nov 26, 2019
@alan-agius4
Copy link
Copy Markdown
Collaborator

Out of curiosity can you check if it fixes also #14095

Comment thread packages/angular_devkit/build_angular/package.json Outdated
@dgp1130
Copy link
Copy Markdown
Collaborator Author

dgp1130 commented Nov 26, 2019

@alan-agius4, I tried out on their repro case and I do see open-iconic in the output. I see it as:

--------------------------------------------------------------------------------
open-iconic v1.1.1 - Iconic
git+https://github.com/iconic/open-iconic.git
--------------------------------------------------------------------------------

(MIT License OR SIL OFL 1.1)

The user said they expected to see that repo's FONT-LICENSE and ICON-LICENSE. I don't think those particular files are picked up, as this seems to come from the package.json. I think that would be expected as those two *-LICENSE names are non-standard. If so, we can probably close that issue as well with this PR.

@alan-agius4
Copy link
Copy Markdown
Collaborator

Thanks @dgp1130 for checking the ionic issue. Will update the issue as suggested.

@dgp1130
Copy link
Copy Markdown
Collaborator Author

dgp1130 commented Dec 3, 2019

Discussed this PR, and we'd rather not change a dependency like this so close to the v9 release.

Decision is to instead suppress the warnings for v9, and then for v9.x we can re-evaluate if we want to try and fix the existing dependency or change to this one.

@alan-agius4 alan-agius4 added the needs: discussion On the agenda for team meeting to determine next steps label Mar 13, 2020
@dgp1130
Copy link
Copy Markdown
Collaborator Author

dgp1130 commented Apr 2, 2020

Now is probably a good time to make this happen, I'll revisit and update this PR.

@dgp1130 dgp1130 removed state: blocked needs: discussion On the agenda for team meeting to determine next steps labels Apr 2, 2020
@dgp1130 dgp1130 force-pushed the third-party branch 3 times, most recently from 8d10ccf to c39ea49 Compare April 2, 2020 22:10
@dgp1130
Copy link
Copy Markdown
Collaborator Author

dgp1130 commented Apr 2, 2020

Rebased the PR on master and double checked that it still appears to be working as expected. I also added a second commit reverting the log suppression code that is no longer necessary. PTAL.

I believe this can merge to patch as I don't think we make guarantees about the format of the 3rdpartylicenses.txt file. Let me know if this should be considered a breaking change and merged only to master for v10.

@alan-agius4
Copy link
Copy Markdown
Collaborator

The revert commit message should be reworded to

revert: "fix(@angular-devkit/build-angular): suppress duplicate 3rdpartylicenses.txt warning

I think since we are changing the dependency and the output, it's safer to target only master.

@alan-agius4 alan-agius4 added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 7, 2020
@alan-agius4 alan-agius4 removed the request for review from clydin April 7, 2020 18:46
@dgp1130 dgp1130 added target: major This PR is targeted for the next major release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Apr 7, 2020
@dgp1130
Copy link
Copy Markdown
Collaborator Author

dgp1130 commented Apr 7, 2020

Fixed the commit message, rebased to master again, and updated the labels to target master only.

@alan-agius4 alan-agius4 added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 8, 2020
Copy link
Copy Markdown
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM, kindly rebase again as you have conflicts.

"sass-loader": "8.0.2",
"semver": "7.1.3",
"source-map": "0.7.3",
"source-map-support": "0.5.16",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oops, please add this back as this is a direct depedency of build-angular.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. Any particular reason this is needed but not detected? I'm not sure how yarn decided to remove this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am aware of any functionality from yarn to remove dependencies.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, strange. I'm not sure why these got removed/reordered then. I think this happened when I did yarn add, but I'm not too sure as it was some months ago when I first made the PR.

Copy link
Copy Markdown
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

See above.

renovate-bot and others added 2 commits April 8, 2020 11:33
…rtylicenses.txt warning"

This reverts commit 59c9802.

No longer necessary to suppress this log, now that the root cause has been fixed.
@dgp1130
Copy link
Copy Markdown
Collaborator Author

dgp1130 commented Apr 8, 2020

I also rebased onto master.

@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 8, 2020
@angular angular deleted a comment from ngbot Bot Apr 8, 2020
@mgechev mgechev merged commit 863067c into angular:master Apr 8, 2020
@dgp1130 dgp1130 deleted the third-party branch April 8, 2020 20:17
@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators May 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WARNING in Conflict: Multiple assets emit different content to the same filename 3rdpartylicenses.txt

6 participants