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

Collision logging, Transformers for JSON and Standard Files #962

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Master-Code-Programmer
Copy link

This is the PR #773, but on a new, rebased branch (the original PR was made 2 years ago).

Hi, I fitted the collsion_logging merge request from chapmajs to your current source code (#126).
Also I followed your wish to use typed parameters (which was really needed, because after adding them, some problems became obvious).

After that I used the modified plugin and with the warnings I could fix my problems instantly. Great!

The only problem was, that I was flooded with warnings about colliding META-INF/NOTICE, META-INF/license.txt, readme.txt and similar files. So I added a transformer, StandardFilesMergeTransformer, which takes these standard files, which exist in all big projects in big numbers, and merges them.
Since these are all pretty primitive files regarding their structure, no special YAML or JSON files, I just concatenate their contents the following way:

  • Standard files are "META-INF/license", "META-INF/notice", "META-INF/readme" or "readme". Which may have the following extensions: none, "txt", "md", "htm" or "html".
  • If a standard file is found (case insensitive), its content is read, stripped of leading and trailing newlines, and then searched in the list of already existing contents of that file. The search is case insensitive.
    • If no existing entry was found a new entry is created and its content and origin is recorded.
    • If an existing entry is found, its list of origins is extended with the origin of the content.
  • Later, the transformed entries are written like that:
    Origins: attoparser-2.0.5.RELEASE.jar, commons-codec-1.15.jar

                                 Apache License
                           Version 2.0, January 2004
    ...
    ================================================================================
    Origins: flyway-core-8.5.4.jar, flyway-mysql-8.5.4.jar

    Copyright (C) Red Gate Software Ltd 2010-2022

    Licensed under the Apache License, Version 2.0 (the "License");
    ...
  • The StandardFilesMergeTransformer transformer is added as default. It can be removed with "removeDefaultTransformers".
    I added it as default because otherwise:

    • In projects with many dependencies the user gets flooded with information about duplicate entries
      like "META-INF/notice.txt", "META-INF/license.txt"...
    • Important licensing information written in META-INF/license.txt and other files may be lost.
    • Helpful information written in readme files may be lost.
    • The merging of this plain text files is safe, there is no important logic to follow. Merged HTML may not look that good, but it works.
  • Added JsonTransformer for JSON files, based on JsonTransformer from @LogicFan. Their code is also under Apache License 2. Extended it, so it can be applied to multiple files.

  • Lowered the logging level for colliding META-INF/MANIFEST.MF files, from warning to debug, so the user doesn't get flooded with unimportant messages.

  • In discussion Shadow Jar for modular project with module-info #710 it came to light that some people need the original module-info.class not to be removed, which is also a possible source for file collision problems. While it's hard to find a nice, quick solution for that, I at least added an option to include the module-info files and list their contents (listing appears only if module-info.class's aren't excluded).
    The option is: allowModuleInfos(), and used in action in def 'Tests the info about the module-info.class, if removed from standard excludes'().

With that changes, any user should be capable to build a fat JAR with more than one Spring (Boot) dependency, right out of the box. Otherwise it's a real pain to find out which files collide, and how to merge colliding JSON files, since that is the only colliding file type, which misses a merging transformer from this plugin.
And JSON is very common!

@Goooler
Copy link
Member

Goooler commented Sep 18, 2024

Sorry for the delay. Can you rebase again? I changed a lot on the main...

@Master-Code-Programmer
Copy link
Author

Sorry for the delay. Can you rebase again? I changed a lot on the main...

Did the rebase.

.gitignore Outdated Show resolved Hide resolved
Comment on lines +113 to +115
if (!shadow.isAllowModuleInfos()) {
excludes.add(MODULE_INFO_CLASS)
}
Copy link
Member

Choose a reason for hiding this comment

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

I see so many changes are addressed in this PR, supporting including module-info.class is one of them, right? Can we extract this point as a separated PR?

Choose a reason for hiding this comment

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

Well, the problem is: Including the module-info.class isn't directly a problem. While it's non-intuitive to include it, it's already possible.
The real problem comes into play if you unknowingly add a dependency which contains also a module-info.class. Meaning: You have a very, really problematic file collision. Which is what this patch is about.

So I added a listing, to inform about the module-info.classs content. But only if the user wants to include it. Because in the default settings, the module-info is excluded anyway.
And after much exploration I gave up, and have to admit that johnrengelman was completely right in #710: The only clean way allow the module-info, is with an extra option.

But I already thought: Maybe I should only list the module-info's content if there is a file collision? On the other hand: That file is so important if included, I decided to list it always.

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

Successfully merging this pull request may close these issues.

3 participants