You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "cfmcgrady (via GitHub)" <gi...@apache.org> on 2023/08/09 11:01:26 UTC
[GitHub] [incubator-celeborn] cfmcgrady opened a new pull request, #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
cfmcgrady opened a new pull request, #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
As title
### Why are the changes needed?
consolidate all sbt dependencies into a global object `Dependencies`, similar to Maven's dependencyManagement, to improve dependency management.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass GA
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cfmcgrady commented on a diff in pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802#discussion_r1289640550
##########
project/CelebornBuild.scala:
##########
@@ -334,14 +383,14 @@ object CelebornWorker {
.settings (
commonSettings,
libraryDependencies ++= Seq(
- "com.google.guava" % "guava" % guavaVersion,
- "commons-io" % "commons-io" % commonsIoVersion,
- "io.netty" % "netty-all" % nettyVersion,
- "org.apache.logging.log4j" % "log4j-1.2-api" % log4j2Version,
- "org.apache.logging.log4j" % "log4j-slf4j-impl" % log4j2Version,
- "org.fusesource.leveldbjni" % "leveldbjni-all" % leveldbJniVersion,
- "org.roaringbitmap" % "RoaringBitmap" % roaringBitmapVersion,
- "org.mockito" %% "mockito-scala-scalatest" % scalatestMockitoVersion % "test",
+ Dependencies.guava,
+ Dependencies.commonsIo,
+ Dependencies.ioNetty,
+ Dependencies.log4j12Api,
Review Comment:
In the current implementation, we're using a naming pattern like this:
```
${dependency}${scope}
```
For instance:
```
`log4j12Api` -> "org.apache.logging.log4j" % "log4j-1.2-api" % log4j2Version
`log4j12ApiTest` -> "org.apache.logging.log4j" % "log4j-1.2-api" % log4j2Version % "test"
```
Moreover, a dependency lacking a scope suffix designates the default runtime scope.
Do you have a better suggestion for the name rules?
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802#discussion_r1291392137
##########
project/CelebornBuild.scala:
##########
@@ -55,20 +43,81 @@ object CelebornCommonSettings {
val guavaVersion = "14.0.1"
val hadoopVersion = "3.2.4"
val javaxServletVersion = "3.1.0"
+ val junitInterfaceVersion = "0.13.3"
+ // don't forget update `junitInterfaceVersion` when we upgrade junit
Review Comment:
I see, it makes sense, thanks for explaination
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cfmcgrady commented on a diff in pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802#discussion_r1293096237
##########
project/CelebornBuild.scala:
##########
@@ -334,14 +383,14 @@ object CelebornWorker {
.settings (
commonSettings,
libraryDependencies ++= Seq(
- "com.google.guava" % "guava" % guavaVersion,
- "commons-io" % "commons-io" % commonsIoVersion,
- "io.netty" % "netty-all" % nettyVersion,
- "org.apache.logging.log4j" % "log4j-1.2-api" % log4j2Version,
- "org.apache.logging.log4j" % "log4j-slf4j-impl" % log4j2Version,
- "org.fusesource.leveldbjni" % "leveldbjni-all" % leveldbJniVersion,
- "org.roaringbitmap" % "RoaringBitmap" % roaringBitmapVersion,
- "org.mockito" %% "mockito-scala-scalatest" % scalatestMockitoVersion % "test",
+ Dependencies.guava,
+ Dependencies.commonsIo,
+ Dependencies.ioNetty,
+ Dependencies.log4j12Api,
Review Comment:
updated
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cfmcgrady commented on a diff in pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802#discussion_r1289649242
##########
project/CelebornBuild.scala:
##########
@@ -334,14 +383,14 @@ object CelebornWorker {
.settings (
commonSettings,
libraryDependencies ++= Seq(
- "com.google.guava" % "guava" % guavaVersion,
- "commons-io" % "commons-io" % commonsIoVersion,
- "io.netty" % "netty-all" % nettyVersion,
- "org.apache.logging.log4j" % "log4j-1.2-api" % log4j2Version,
- "org.apache.logging.log4j" % "log4j-slf4j-impl" % log4j2Version,
- "org.fusesource.leveldbjni" % "leveldbjni-all" % leveldbJniVersion,
- "org.roaringbitmap" % "RoaringBitmap" % roaringBitmapVersion,
- "org.mockito" %% "mockito-scala-scalatest" % scalatestMockitoVersion % "test",
+ Dependencies.guava,
+ Dependencies.commonsIo,
+ Dependencies.ioNetty,
+ Dependencies.log4j12Api,
Review Comment:
Another idea is we only define the runtime dep within the object Dependencies. Whenever we want to adjust the scope of a dependency, we would then add the scope as needed.
```scala
Dependencies.log4j12Api % "test"
```
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802#discussion_r1289478070
##########
project/CelebornBuild.scala:
##########
@@ -334,14 +383,14 @@ object CelebornWorker {
.settings (
commonSettings,
libraryDependencies ++= Seq(
- "com.google.guava" % "guava" % guavaVersion,
- "commons-io" % "commons-io" % commonsIoVersion,
- "io.netty" % "netty-all" % nettyVersion,
- "org.apache.logging.log4j" % "log4j-1.2-api" % log4j2Version,
- "org.apache.logging.log4j" % "log4j-slf4j-impl" % log4j2Version,
- "org.fusesource.leveldbjni" % "leveldbjni-all" % leveldbJniVersion,
- "org.roaringbitmap" % "RoaringBitmap" % roaringBitmapVersion,
- "org.mockito" %% "mockito-scala-scalatest" % scalatestMockitoVersion % "test",
+ Dependencies.guava,
+ Dependencies.commonsIo,
+ Dependencies.ioNetty,
+ Dependencies.log4j12Api,
Review Comment:
it's not clear to me the scope of each dep, and there is possible that a dep may have different scopes in different modules
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] codecov[bot] commented on pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802#issuecomment-1671129553
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1802?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1802](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1802?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (92408a4) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/0d1261632dca9d67a1684cb22c1e92d7da6bb5d7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (0d12616) will **decrease** coverage by `0.19%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #1802 +/- ##
==========================================
- Coverage 46.76% 46.57% -0.19%
==========================================
Files 162 162
Lines 10077 10077
Branches 927 927
==========================================
- Hits 4712 4692 -20
- Misses 5058 5073 +15
- Partials 307 312 +5
```
[see 2 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1802/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cfmcgrady commented on a diff in pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802#discussion_r1289640550
##########
project/CelebornBuild.scala:
##########
@@ -334,14 +383,14 @@ object CelebornWorker {
.settings (
commonSettings,
libraryDependencies ++= Seq(
- "com.google.guava" % "guava" % guavaVersion,
- "commons-io" % "commons-io" % commonsIoVersion,
- "io.netty" % "netty-all" % nettyVersion,
- "org.apache.logging.log4j" % "log4j-1.2-api" % log4j2Version,
- "org.apache.logging.log4j" % "log4j-slf4j-impl" % log4j2Version,
- "org.fusesource.leveldbjni" % "leveldbjni-all" % leveldbJniVersion,
- "org.roaringbitmap" % "RoaringBitmap" % roaringBitmapVersion,
- "org.mockito" %% "mockito-scala-scalatest" % scalatestMockitoVersion % "test",
+ Dependencies.guava,
+ Dependencies.commonsIo,
+ Dependencies.ioNetty,
+ Dependencies.log4j12Api,
Review Comment:
In the current implementation, we're using a naming pattern like this:
```
${dependency}${scope}
```
For instance:
```
log4j12Api -> "org.apache.logging.log4j" % "log4j-1.2-api" % log4j2Version
log4j12ApiTest -> "org.apache.logging.log4j" % "log4j-1.2-api" % log4j2Version % "test"
```
Moreover, a dependency lacking a scope suffix designates the default runtime scope.
Do you have a better suggestion for the name rules?
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cfmcgrady commented on pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on PR #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802#issuecomment-1679880590
thanks, merging to main(v0.4.0).
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802#discussion_r1291390465
##########
project/CelebornBuild.scala:
##########
@@ -334,14 +383,14 @@ object CelebornWorker {
.settings (
commonSettings,
libraryDependencies ++= Seq(
- "com.google.guava" % "guava" % guavaVersion,
- "commons-io" % "commons-io" % commonsIoVersion,
- "io.netty" % "netty-all" % nettyVersion,
- "org.apache.logging.log4j" % "log4j-1.2-api" % log4j2Version,
- "org.apache.logging.log4j" % "log4j-slf4j-impl" % log4j2Version,
- "org.fusesource.leveldbjni" % "leveldbjni-all" % leveldbJniVersion,
- "org.roaringbitmap" % "RoaringBitmap" % roaringBitmapVersion,
- "org.mockito" %% "mockito-scala-scalatest" % scalatestMockitoVersion % "test",
+ Dependencies.guava,
+ Dependencies.commonsIo,
+ Dependencies.ioNetty,
+ Dependencies.log4j12Api,
Review Comment:
I like the latter one
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802#discussion_r1289477050
##########
project/CelebornBuild.scala:
##########
@@ -55,20 +43,81 @@ object CelebornCommonSettings {
val guavaVersion = "14.0.1"
val hadoopVersion = "3.2.4"
val javaxServletVersion = "3.1.0"
+ val junitInterfaceVersion = "0.13.3"
+ // don't forget update `junitInterfaceVersion` when we upgrade junit
Review Comment:
seems no strong contract?
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cfmcgrady commented on a diff in pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #1802:
URL: https://github.com/apache/incubator-celeborn/pull/1802#discussion_r1289632612
##########
project/CelebornBuild.scala:
##########
@@ -55,20 +43,81 @@ object CelebornCommonSettings {
val guavaVersion = "14.0.1"
val hadoopVersion = "3.2.4"
val javaxServletVersion = "3.1.0"
+ val junitInterfaceVersion = "0.13.3"
+ // don't forget update `junitInterfaceVersion` when we upgrade junit
Review Comment:
1. The JUnit interface delineates its JUnit version compatibility matrix within its [documentation](https://github.com/sbt/junit-interface/#setup). Therefore, I think it's best to keep it consistent with the documentation.
| junit-interface version | JUnit version |
|------------------------|---------------|
| 0.13.2 | 4.13.2 |
| 0.13.1 | 4.13.1 |
| 0.13 | 4.13 |
| 0.12 | 4.12 |
2. As per the [SBT documentation](https://www.scala-sbt.org/1.x/docs/Testing.html#JUnit), if we're planning to upgrade from JUnit 4 to JUnit 5, we'll need to switch from using the sbt JUnit interface to the sbt-jupiter-interface.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cfmcgrady closed pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady closed pull request #1802: [CELEBORN-884][BUILD] Consolidate all dependencies into a global object `Dependencies`
URL: https://github.com/apache/incubator-celeborn/pull/1802
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org