[CELEBORN-1031] SBT correct the LICENSE and NOTICE for shaded client jars #1967

cxzl25 · 2023-10-10T04:43:18Z

What changes were proposed in this pull request?

Flink/Spark jars packaged with SBT use the correct LICENSE and NOTICE.

Why are the changes needed?

#1930 (comment)

Does this PR introduce any user-facing change?

How was this patch tested?

codecov · 2023-10-10T04:55:59Z

Codecov Report

Merging #1967 (1ad6634) into main (4465a92) will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1967      +/-   ##
==========================================
+ Coverage   46.71%   46.84%   +0.14%     
==========================================
  Files         165      165              
  Lines       10668    10668              
  Branches      972      972              
==========================================
+ Hits         4982     4996      +14     
+ Misses       5362     5350      -12     
+ Partials      324      322       -2

see 2 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

cxzl25 · 2023-10-12T13:22:24Z

cc @pan3793 @cfmcgrady

pan3793 · 2023-10-12T13:24:10Z

project/CelebornBuild.scala

+          case m if m == "META-INF/LICENSE.txt" => MergeStrategy.discard
+          case m if m == "META-INF/NOTICE.txt" => MergeStrategy.discard
+          case m if m == "LICENSE.txt" => MergeStrategy.discard
+          case m if m == "NOTICE.txt" => MergeStrategy.discard


is it possible to know which jar the file comes from?

the ideal goal here is to drop all files **/LICENSE.* and **/NOTICE.* that come from the third-party jars and only keep the one provided by ourselves. I don't find an elegant approach in Maven, hope we can make it happen in SBT

maven-shade-plugin provides some license transformers.

https://maven.apache.org/plugins/maven-shade-plugin/examples/resource-transformers.html

<transformer implementation="org.apache.maven.plugins.shade.resource.ApacheLicenseResourceTransformer"/> <transformer implementation="org.apache.maven.plugins.shade.resource.ApacheNoticeResourceTransformer"/>

Implemented a simple ResourceTransformer that can output LICENSE.txt and NOTICE.txt source jar.

import org.apache.maven.plugins.shade.resource.ResourceTransformer; public class PrintResourceTransformer implements ResourceTransformer { @Override public boolean canTransformResource(String s) { boolean hasFlag = false; if (s.contains("LICENSE.txt")) { hasFlag = true; } else if (s.contains("NOTICE.txt")) { hasFlag = true; } if (hasFlag) { URL resource = this.getClass().getResource("/" + s); System.err.printf("PrintResourceTransformer %s,%s,%s%n", s, hasFlag, resource); } return hasFlag; }

PrintResourceTransformer META-INF/LICENSE.txt,true,jar:file:/Users/csy/.m2/repository/org/jdom/jdom2/2.0.6.1/jdom2-2.0.6.1.jar!/META-INF/LICENSE.txt PrintResourceTransformer META-INF/NOTICE.txt,true,jar:file:/Users/csy/.m2/repository/commons-io/commons-io/2.6/commons-io-2.6.jar!/META-INF/NOTICE.txt

unfortunately, the built-in apache license/notice transformer does not work well during my previous test

unfortunately, the built-in apache license/notice transformer does not work well during my previous test

ApacheLicenseResourceTransformer just ignores META-INF/LICENSE, META-INF/LICENSE.txt, META-INF/LICENSE.md.

https://github.com/apache/maven-shade-plugin/blob/master/src/main/java/org/apache/maven/plugins/shade/resource/ApacheLicenseResourceTransformer.java#L31

@cxzl25 can we implement it in sbt? the previous discussion aims to remove maven and only keep sbt eventually.

the ideal goal here is to drop all files **/LICENSE.* and **/NOTICE.* that come from the third-party jars and only keep the one provided by ourselves. I don't find an elegant approach in Maven, hope we can make it happen in SBT

we can refer to pekko, and achieve this goal with MergeStrategy.concat

assembly / assemblyMergeStrategy := { case n if n.endsWith(".class") => MergeStrategy.first case n if n.endsWith(".txt") => MergeStrategy.concat case n if n.endsWith("NOTICE") => MergeStrategy.concat case n if n.endsWith("LICENSE") => MergeStrategy.concat case n => (assembly / assemblyMergeStrategy).value.apply(n) },

How about using contains? I tested it locally and verified that it can be excluded.

cfmcgrady · 2023-10-16T06:12:31Z

I wrote a script and found that SBT's LICENSE and NOTICE are consistent with Maven's.

#!/bin/bash

# Specify the paths for two directories
dir1="sbt"
dir2="maven"

# Use find command to list all files in the first directory
find "$dir1" -type f | while read file1; do
    # Get the relative path
    rel_path="${file1#$dir1}"
    # Construct the corresponding file path in the second directory
    file2="$dir2$rel_path"

    # Check if a file with the same path exists in the second directory
    if [ -e "$file2" ]; then
        # Use md5sum command to compare contents of the two files
        hash1=$(md5sum "$file1" | awk '{print $1}')
        hash2=$(md5sum "$file2" | awk '{print $1}')

        # Compare MD5 hash values
        if [[ "$file1" != *.class && "$hash1" != "$hash2" ]]; then
            echo "Files differ: $file1 and $file2"
        fi
    else
        # Print file path if it doesn't exist in second directory
        echo "File does not exist in second directory: $file1"
    fi
done

# Check if there are files in the second directory not in the first
find "$dir2" -type f | while read file2; do
    # Get the relative path
    rel_path="${file2#$dir2}"
    # Construct the corresponding file path in the first directory
    file1="$dir1$rel_path"

    # Print file path if it doesn't exist in first directory
    if [ ! -e "$file1" ]; then
        echo "File does not exist in first directory: $file2"
    fi
done

Files differ: sbt/celeborn-client-spark-3-shaded_2.12-0.4.0-SNAPSHOT.jar and maven/celeborn-client-spark-3-shaded_2.12-0.4.0-SNAPSHOT.jar
File does not exist in second directory: sbt/org/apache/celeborn/package$.class
File does not exist in second directory: sbt/org/apache/celeborn/package.class
Files differ: sbt/META-INF/services/reactor.blockhound.integration.BlockHoundIntegration and maven/META-INF/services/reactor.blockhound.integration.BlockHoundIntegration
Files differ: sbt/META-INF/MANIFEST.MF and maven/META-INF/MANIFEST.MF
File does not exist in second directory: sbt/META-INF/NOTICE.txt
File does not exist in second directory: sbt/META-INF/LICENSE.txt
File does not exist in second directory: sbt/META-INF/INDEX.LIST
File does not exist in first directory: maven/META-INF/maven/org.apache.celeborn/celeborn-common_2.12/pom.xml
File does not exist in first directory: maven/META-INF/maven/org.apache.celeborn/celeborn-common_2.12/pom.properties
File does not exist in first directory: maven/META-INF/maven/org.apache.celeborn/celeborn-client_2.12/pom.xml
File does not exist in first directory: maven/META-INF/maven/org.apache.celeborn/celeborn-client_2.12/pom.properties
File does not exist in first directory: maven/META-INF/maven/org.apache.celeborn/celeborn-client-spark-3-shaded_2.12/pom.xml
File does not exist in first directory: maven/META-INF/maven/org.apache.celeborn/celeborn-client-spark-3-shaded_2.12/pom.properties
File does not exist in first directory: maven/META-INF/maven/org.apache.celeborn/celeborn-client-spark-common_2.12/pom.xml
File does not exist in first directory: maven/META-INF/maven/org.apache.celeborn/celeborn-client-spark-common_2.12/pom.properties
File does not exist in first directory: maven/META-INF/maven/org.apache.celeborn/celeborn-client-spark-3_2.12/pom.xml
File does not exist in first directory: maven/META-INF/maven/org.apache.celeborn/celeborn-client-spark-3_2.12/pom.properties
File does not exist in first directory: maven/META-INF/DEPENDENCIES

pan3793 · 2023-10-16T07:42:27Z

I wrote a script and found that SBT's LICENSE and NOTICE are consistent with Maven's.

Thanks for the effort, actually, as mentioned above, I don't think the current implementation in Maven is perfect.

The goals of this part are:

filter out all LICENSE.* and NOTICE.* from the third-party jars, no matter from which folders.
only include the LICENSE and NOTICE we provide.

cxzl25 · 2023-11-01T05:35:58Z

project/CelebornBuild.scala

@@ -679,6 +691,8 @@ trait SparkClientProjects {

        (assembly / assemblyMergeStrategy) := {
          case m if m.toLowerCase(Locale.ROOT).endsWith("manifest.mf") => MergeStrategy.discard
+          case m if m.startsWith("META-INF/license/") => MergeStrategy.discard
+          case PathList(ps@_*) if Assembly.isLicenseFile(ps.last) => licenseFileOnlyProjectMergeStrategy(ps.mkString(java.io.File.separator))


sbtassembly.Assembly#isLicenseFile

/** * Returns a flag if the given file is a license file * * @param fileName the given file to check * @return flag representing if the file is a license file */ def isLicenseFile(fileName: String): Boolean = { val LicenseFile = """(license|licence|notice|copying)([.]\w+)?$""".r fileName.toLowerCase match { case LicenseFile(_, ext) if ext != ".class" => true // DISLIKE case _ => false } }

cxzl25 · 2023-11-01T05:36:50Z

project/CelebornBuild.scala

@@ -205,6 +205,18 @@ object CelebornCommonSettings {
    Dependencies.junit % "test",
    // https://www.scala-sbt.org/1.x/docs/Testing.html
    Dependencies.junitInterface % "test")
+
+  def licenseFileOnlyProjectMergeStrategy(path: String) =


./build/make-distribution.sh --sbt-enabled

META-INF/LICENSE from Project dependency: Project name = celeborn-client-spark-3-shaded_2.12-0.4.0-SNAPSHOT, target = META-INF/LICENSE [Discard] META-INF/NOTICE.txt Not from Project dependency: Jar name = commons-lang3-3.12.0.jar, jar org = org.apache.commons, entry target = META-INF/NOTICE.txt [Discard] META-INF/LICENSE.txt Not from Project dependency: Jar name = commons-lang3-3.12.0.jar, jar org = org.apache.commons, entry target = META-INF/LICENSE.txt META-INF/NOTICE from Project dependency: Project name = celeborn-client-spark-3-shaded_2.12-0.4.0-SNAPSHOT, target = META-INF/NOTICE

pan3793 · 2023-11-01T05:55:51Z

project/CelebornBuild.scala

+          println(s"$path from Project dependency: $dep")
+          Some(JarEntry(dep.target, dep.stream))
+        case dep =>
+          println(s"[Discard] $path Not from Project dependency: $dep")


I roughly remember the sbt has a built-in slf4j logger

https://github.com/apache/incubator-toree/blob/master/build.sbt#L33C1

how about

assembly - including file: celeborn-client-spark-3-shaded_2.12:META-INF/LICENSE assembly - excluding file: commons-lang3-3.12.0.jar:META-INF/LICENSE.txt

?

cxzl25 · 2023-11-01T07:30:00Z

project/CelebornBuild.scala

@@ -205,6 +205,18 @@ object CelebornCommonSettings {
    Dependencies.junit % "test",
    // https://www.scala-sbt.org/1.x/docs/Testing.html
    Dependencies.junitInterface % "test")
+
+  def licenseFileOnlyProjectMergeStrategy(log: Logger) =


[info] assembly - excluding file: Jar name = commons-lang3-3.12.0.jar, jar org = org.apache.commons, entry target = META-INF/LICENSE.txt [info] assembly - excluding file: Jar name = commons-lang3-3.12.0.jar, jar org = org.apache.commons, entry target = META-INF/NOTICE.txt [info] assembly - including file: META-INF/NOTICE [info] assembly - including file: META-INF/LICENSE

I think the built-in strategy MergeStrategy.preferProject can achieve this goal

merge strategy

MergeStrategy.preferProject will choose the first project file over library files if present. Otherwise, it works like MergeStrategy.first

Maybe not? If our project does not have the same file

MergeStrategy.preferProject

https://github.com/sbt/sbt-assembly/blob/e4d3b362997a32da21d97c051c9d92ecc5cdb9c2/src/main/scala/sbtassembly/MergeStrategy.scala#L82-L88

sbtassembly.MergeStrategy#preferProject

/** * Picks the dependency that is the first project/internal dependency, otherwise picks the first library/external dependency */ val preferProject: MergeStrategy = MergeStrategy("PreferProject") { conflicts => val entry = conflicts.find(_.isProjectDependency).getOrElse(conflicts.head) Right(Vector(JarEntry(entry.target, entry.stream))) }

use CustomMergeStrategy

is it feasible to combine using MergeStrategy.preferProject and MergeStrategy.discard?

case "LICENSE" | "NOTICE" => MergeStrategy.preferProject case m if Assembly.isLicenseFile(m) => MergeStrategy.discard

case PathList(ps@_*) if ps.last == "LICENSE" || ps.last == "NOTICE" => MergeStrategy.preferProject case PathList(ps@_*) if Assembly.isLicenseFile(ps.last) => MergeStrategy.discard

cfmcgrady · 2023-11-02T14:10:15Z

project/CelebornBuild.scala

@@ -870,6 +885,8 @@ trait FlinkClientProjects {

        (assembly / assemblyMergeStrategy) := {
          case m if m.toLowerCase(Locale.ROOT).endsWith("manifest.mf") => MergeStrategy.discard
+          case m if m.startsWith("META-INF/license/") => MergeStrategy.discard


why do we need to discard the files in folder META-INF/license/?

The MR module depends on the netty 3x version, so there is a META-INF/license/ directory
Check that Spark and Flink do not depend on netty 3x. This configuration is now removed.

https://repo1.maven.org/maven2/io/netty/netty/3.10.6.Final/netty-3.10.6.Final.jar

cfmcgrady · 2023-11-05T06:44:14Z

$ ./build/sbt -Pspark-3.3 celeborn-client-spark-3-shaded/assembly
$ jar tf client-spark/spark-3-shaded/target/scala-2.12/celeborn-client-spark-3-shaded_2.12-0.4.0-SNAPSHOT.jar | grep -i 'license\|notice'
META-INF/LICENSE
META-INF/NOTICE
META-INF/licenses/
META-INF/licenses/LICENSE-protobuf.txt

cfmcgrady · 2023-11-06T03:11:36Z

project/CelebornBuild.scala

-          case m if m == "META-INF/NOTICE.txt" => MergeStrategy.discard
-          case m if m == "LICENSE.txt" => MergeStrategy.discard
-          case m if m == "NOTICE.txt" => MergeStrategy.discard
+          case PathList(ps@_*) if ps.last == "LICENSE" || ps.last == "NOTICE" => MergeStrategy.preferProject


Suggested change

case PathList(ps@_*) if ps.last == "LICENSE" || ps.last == "NOTICE" => MergeStrategy.preferProject

case "LICENSE" | "NOTICE" => MergeStrategy.discard

case PathList(ps@_*) if ps.last == "LICENSE" || ps.last == "NOTICE" => MergeStrategy.preferProject

we should discard the NOTICE/LICENSE files from the scala-library for mr-shade client.

$ ./build/sbt -Pmr celeborn-client-mr-shaded/assembly $ jar tf client-mr/mr-shaded/target/scala-2.12/celeborn-client-mr-shaded_2.12-0.4.0-SNAPSHOT.jar | grep -i 'license\|notice' LICENSE META-INF/LICENSE META-INF/NOTICE META-INF/licenses/ META-INF/licenses/LICENSE-protobuf.txt NOTICE

Thanks for finding this problem.
I have now changed to use preferProject only for META-INF/LICENSE and META-INF/NOTICE.

project/CelebornBuild.scala

cfmcgrady · 2023-11-06T06:17:03Z

thanks, merging to main(v0.4.0).

pan3793 reviewed Oct 12, 2023

View reviewed changes

cfmcgrady approved these changes Oct 16, 2023

View reviewed changes

cxzl25 commented Nov 1, 2023

View reviewed changes

pan3793 reviewed Nov 1, 2023

View reviewed changes

pan3793 approved these changes Nov 1, 2023

View reviewed changes

cxzl25 commented Nov 1, 2023

View reviewed changes

pan3793 requested a review from cfmcgrady November 1, 2023 08:15

cfmcgrady reviewed Nov 2, 2023

View reviewed changes

cxzl25 force-pushed the CELEBORN-1031 branch from 3036825 to 000d9f4 Compare November 3, 2023 03:13

cxzl25 added 7 commits November 5, 2023 10:54

license

19270c6

contains

098056a

custom strategy

40bd06e

use sLog

bd7f4d2

remove path

2f0ec39

META-INF/license/

e7fecb4

preferProject

acf13b4

cxzl25 force-pushed the CELEBORN-1031 branch from a33c0dc to acf13b4 Compare November 5, 2023 02:57

cfmcgrady approved these changes Nov 6, 2023

View reviewed changes

cfmcgrady requested a review from pan3793 November 6, 2023 02:41

cfmcgrady reviewed Nov 6, 2023

View reviewed changes

discard root path license file

9ae48b9

cfmcgrady reviewed Nov 6, 2023

View reviewed changes

project/CelebornBuild.scala Outdated Show resolved Hide resolved

Update project/CelebornBuild.scala

4db8623

cfmcgrady reviewed Nov 6, 2023

View reviewed changes

project/CelebornBuild.scala Outdated Show resolved Hide resolved

Update project/CelebornBuild.scala

cd2db07

cfmcgrady reviewed Nov 6, 2023

View reviewed changes

project/CelebornBuild.scala Outdated Show resolved Hide resolved

Update project/CelebornBuild.scala

1ad6634

cfmcgrady approved these changes Nov 6, 2023

View reviewed changes

cfmcgrady closed this in 23d7c20 Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CELEBORN-1031] SBT correct the LICENSE and NOTICE for shaded client jars #1967

[CELEBORN-1031] SBT correct the LICENSE and NOTICE for shaded client jars #1967

cxzl25 commented Oct 10, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading

cxzl25 commented Oct 12, 2023

pan3793 Oct 12, 2023

pan3793 Oct 12, 2023 •

edited

Loading

cxzl25 Oct 12, 2023

cxzl25 Oct 12, 2023

pan3793 Oct 12, 2023

cxzl25 Oct 16, 2023

pan3793 Oct 16, 2023 •

edited

Loading

cfmcgrady Oct 16, 2023

cxzl25 Oct 25, 2023

cfmcgrady commented Oct 16, 2023

pan3793 commented Oct 16, 2023 •

edited

Loading

cxzl25 Nov 1, 2023

cxzl25 Nov 1, 2023

pan3793 Nov 1, 2023

pan3793 Nov 1, 2023

pan3793 Nov 1, 2023 •

edited

Loading

cxzl25 Nov 1, 2023

cfmcgrady Nov 2, 2023

cxzl25 Nov 2, 2023 •

edited

Loading

pan3793 Nov 3, 2023

cxzl25 Nov 3, 2023

cfmcgrady Nov 2, 2023

cxzl25 Nov 3, 2023

cfmcgrady commented Nov 5, 2023

cfmcgrady Nov 6, 2023

cfmcgrady Nov 6, 2023

cxzl25 Nov 6, 2023

cfmcgrady commented Nov 6, 2023

	case PathList(ps@_*) if ps.last == "LICENSE" \|\| ps.last == "NOTICE" => MergeStrategy.preferProject
	case "LICENSE" \| "NOTICE" => MergeStrategy.discard
	case PathList(ps@_*) if ps.last == "LICENSE" \|\| ps.last == "NOTICE" => MergeStrategy.preferProject

[CELEBORN-1031] SBT correct the LICENSE and NOTICE for shaded client jars #1967

[CELEBORN-1031] SBT correct the LICENSE and NOTICE for shaded client jars #1967

Conversation

cxzl25 commented Oct 10, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

cxzl25 commented Oct 12, 2023

Choose a reason for hiding this comment

pan3793 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfmcgrady commented Oct 16, 2023

pan3793 commented Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cxzl25 Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

MergeStrategy.preferProject

use CustomMergeStrategy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfmcgrady commented Nov 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfmcgrady commented Nov 6, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading

pan3793 Oct 12, 2023 •

edited

Loading

pan3793 Oct 16, 2023 •

edited

Loading

pan3793 commented Oct 16, 2023 •

edited

Loading

pan3793 Nov 1, 2023 •

edited

Loading

cxzl25 Nov 2, 2023 •

edited

Loading