You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/10/13 12:50:09 UTC
[GitHub] [spark] HyukjinKwon opened a new pull request, #38240: [SPARK-40654][PROTOBUF][FOLLOW-UP] Clean up SBT build in Proto component
HyukjinKwon opened a new pull request, #38240:
URL: https://github.com/apache/spark/pull/38240
### What changes were proposed in this pull request?
This PR cleans up the syntax, and properly copy protobuf assembly jar (currently it copies connect assembly jar by mistake).
### Why are the changes needed?
For consistent code style, and correct SBT build.
### Does this PR introduce _any_ user-facing change?
No, this isn't released yet.
### How was this patch tested?
CI in this PR should test it out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #38240: [SPARK-40654][PROTOBUF][FOLLOW-UP] Clean up SBT build in Proto component
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #38240:
URL: https://github.com/apache/spark/pull/38240#issuecomment-1278377305
Merged to master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #38240: [SPARK-40654][PROTOBUF][FOLLOW-UP] Clean up SBT build in Proto component
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38240:
URL: https://github.com/apache/spark/pull/38240#issuecomment-1277846280
hmm... should we add `assemblyPackageScala` and `assemblyExcludedJars` to `SparkProtobuf` module, the assembly jar is a little fat
```
[debug] Including from cache: spark-tags_2.12-3.4.0-SNAPSHOT.jar
[debug] Including from cache: unused-1.0.0.jar
[debug] Including from cache: spark-protobuf_2.12-3.4.0-SNAPSHOT.jar
[debug] Including from cache: scala-collection-compat_2.12-2.2.0.jar
[debug] Including from cache: pmml-model-1.4.8.jar
[debug] Including from cache: protobuf-java-3.21.1.jar
[debug] Including from cache: guava-14.0.1.jar
[debug] Including from cache: scala-library-2.12.17.jar
```
for example, similar as `SparkConnect` module:
```
// Exclude `scala-library` from assembly.
(assembly / assemblyPackageScala / assembleArtifact) := false,
// Exclude `pmml-model-*.jar`, `scala-collection-compat_*.jar`,
// `spark-tags_*.jar`, "guava-*.jar" and `unused-1.0.0.jar` from assembly.
(assembly / assemblyExcludedJars) := {
val cp = (assembly / fullClasspath).value
cp filter { v =>
val name = v.data.getName
name.startsWith("pmml-model-") || name.startsWith("scala-collection-compat_") ||
name.startsWith("spark-tags_") || name.startsWith("guava-") || name == "unused-1.0.0.jar"
}
},
```
then the assembly jar just includes:
```
[debug] Including: spark-protobuf_2.12-3.4.0-SNAPSHOT.jar
[debug] Including from cache: protobuf-java-3.21.1.jar
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #38240: [SPARK-40654][PROTOBUF][FOLLOW-UP] Clean up SBT build in Proto component
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #38240:
URL: https://github.com/apache/spark/pull/38240#issuecomment-1278377195
Let me get this in first in any event since it works (and the built is technically broken without this change).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on a diff in pull request #38240: [SPARK-40654][PROTOBUF][FOLLOW-UP] Clean up SBT build in Proto component
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38240:
URL: https://github.com/apache/spark/pull/38240#discussion_r995214886
##########
project/SparkBuild.scala:
##########
@@ -703,32 +703,24 @@ object SparkConnect {
}
object SparkProtobuf {
-
import BuildCommons.protoVersion
- private val shadePrefix = "org.sparkproject.spark-protobuf"
- val shadeJar = taskKey[Unit]("Shade the Jars")
-
lazy val settings = Seq(
// Setting version for the protobuf compiler. This has to be propagated to every sub-project
// even if the project is not using it.
PB.protocVersion := BuildCommons.protoVersion,
// For some reason the resolution from the imported Maven build does not work for some
// of these dependendencies that we need to shade later on.
- libraryDependencies ++= Seq(
- "com.google.protobuf" % "protobuf-java" % protoVersion % "protobuf"
- ),
+ libraryDependencies += "com.google.protobuf" % "protobuf-java" % protoVersion % "protobuf",
- dependencyOverrides ++= Seq(
- "com.google.protobuf" % "protobuf-java" % protoVersion
- ),
+ dependencyOverrides += "com.google.protobuf" % "protobuf-java" % protoVersion,
(Compile / PB.targets) := Seq(
- PB.gens.java -> (Compile / sourceManaged).value,
+ PB.gens.java -> (Compile / sourceManaged).value,
),
- (assembly / test) := false,
+ (assembly / test) := { },
Review Comment:
just for my own education, is this the same: `false` -> `{ }`?
looks like a bit magic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #38240: [SPARK-40654][PROTOBUF][FOLLOW-UP] Clean up SBT build in Proto component
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #38240:
URL: https://github.com/apache/spark/pull/38240#discussion_r995262265
##########
project/SparkBuild.scala:
##########
@@ -703,32 +703,24 @@ object SparkConnect {
}
object SparkProtobuf {
-
import BuildCommons.protoVersion
- private val shadePrefix = "org.sparkproject.spark-protobuf"
- val shadeJar = taskKey[Unit]("Shade the Jars")
-
lazy val settings = Seq(
// Setting version for the protobuf compiler. This has to be propagated to every sub-project
// even if the project is not using it.
PB.protocVersion := BuildCommons.protoVersion,
// For some reason the resolution from the imported Maven build does not work for some
// of these dependendencies that we need to shade later on.
- libraryDependencies ++= Seq(
- "com.google.protobuf" % "protobuf-java" % protoVersion % "protobuf"
- ),
+ libraryDependencies += "com.google.protobuf" % "protobuf-java" % protoVersion % "protobuf",
- dependencyOverrides ++= Seq(
- "com.google.protobuf" % "protobuf-java" % protoVersion
- ),
+ dependencyOverrides += "com.google.protobuf" % "protobuf-java" % protoVersion,
(Compile / PB.targets) := Seq(
- PB.gens.java -> (Compile / sourceManaged).value,
+ PB.gens.java -> (Compile / sourceManaged).value,
),
- (assembly / test) := false,
+ (assembly / test) := { },
Review Comment:
Yeah, they are same. I believe `{ }` is much more common (googled a bit)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #38240: [SPARK-40654][PROTOBUF][FOLLOW-UP] Clean up SBT build in Proto component
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #38240: [SPARK-40654][PROTOBUF][FOLLOW-UP] Clean up SBT build in Proto component
URL: https://github.com/apache/spark/pull/38240
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org