You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "isaacreath (via GitHub)" <gi...@apache.org> on 2024/12/04 22:53:29 UTC
[PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
isaacreath opened a new pull request, #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155
This PR updates the Gradle version for cassandra-sidecar to the latest stable Gradle release (8.11).
A couple of things to note:
First, the generated Windows start script (`cassandra-sidecar.bat`) does not work on trunk. It fails with the following error message:
```
> .\bin\cassandra-sidecar.bat
Error opening zip file or JAR manifest missing : APP_HOME_TO_REPLACE/agents/jolokia-jvm-1.6.0-agent.jar
Error occurred during initialization of VM
agent library failed to init: instrument
```
The changes I made in https://github.com/apache/cassandra-sidecar/compare/trunk...isaacreath:CASSSIDECAR-167?expand=1#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R129-R135 do not fix the Windows start script. I'm open to disabling the generation of this start script in this PR, but I think to get it actually working properly would be outside of the scope of this change.
Second, the use of `${APP_HOME}` as a default JVM option in the generated start script is discouraged in modern versions of Gradle and is not possible to do out of the box with Gradle 7.2+ (hence the hacks on https://github.com/apache/cassandra-sidecar/compare/trunk...isaacreath:CASSSIDECAR-167?expand=1#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R113-R135). My goal was to ensure as much compatibility with how the `bin/cassandra-sidecar` script was working today, but as a follow up I think we should probably reconsider using it in the default JVM options.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "isaacreath (via GitHub)" <gi...@apache.org>.
isaacreath commented on PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#issuecomment-2537415946
Got CI passing: https://app.circleci.com/pipelines/circleci/2pKxPuEmc4jySZHiGcJrJE/LTM5bKmo57v3FcPSxgh5mk/4/workflows/8f21d3e0-5548-4647-b82e-7e5970b4e519
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "isaacreath (via GitHub)" <gi...@apache.org>.
isaacreath commented on PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#issuecomment-2520482244
Yeah I agree, I'll update this PR to ensure that Gradle does not generate a bat file.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "isaacreath (via GitHub)" <gi...@apache.org>.
isaacreath commented on code in PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#discussion_r1880665830
##########
build.gradle:
##########
@@ -38,11 +38,11 @@ plugins {
id 'application'
// since we're using a specific version here, we delay applying the plugin till the all projects
- id "com.github.spotbugs" version "3.0.0" apply false
+ id "com.github.spotbugs" version "5.0.14" apply false
Review Comment:
I'm guessing you mean `6.0.20` not `6.20.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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "bbotella (via GitHub)" <gi...@apache.org>.
bbotella commented on code in PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#discussion_r1872358638
##########
build.gradle:
##########
@@ -102,18 +105,35 @@ application {
// Take the application out once we're running via Cassandra
mainClassName = "org.apache.cassandra.sidecar.CassandraSidecarDaemon"
applicationName = 'cassandra-sidecar'
- // Config file location should be in file:/// format for local files,
- def confFile = "file:" + File.separator + File.separator + "APP_HOME_TO_REPLACE/conf/sidecar.yaml"
- applicationDefaultJvmArgs = ["-Dsidecar.logdir=./logs",
- "-Dsidecar.config=" + confFile,
- "-Dlogback.configurationFile=./conf/logback.xml",
- "-Dvertx.logger-delegate-factory-class-name=io.vertx.core.logging.SLF4JLogDelegateFactory",
- "-javaagent:APP_HOME_TO_REPLACE/agents/jolokia-jvm-1.6.0-agent.jar=port=7777,host=localhost"]
+ applicationDefaultJvmArgs = ["PLACEHOLDER"]
Review Comment:
Just for my education, what is this placeholder here and on line 134?
##########
gradle/wrapper/gradle-wrapper.properties:
##########
@@ -1,5 +1,5 @@
#Fri May 01 11:14:42 PDT 2020
-distributionUrl=https\://services.gradle.org/distributions/gradle-6.2.1-all.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-8.11-all.zip
Review Comment:
Now that we are on it, should we go with 8.11.1 newer version?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "bbotella (via GitHub)" <gi...@apache.org>.
bbotella commented on PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#issuecomment-2520522278
Thanks a lot for the patch!
I will take a look at it today as well (I'm not a committer, so we will still need @frankgh).
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "isaacreath (via GitHub)" <gi...@apache.org>.
isaacreath commented on PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#issuecomment-2537356341
> The PR looks good to me. Let me know when you have CI results ready so we can work on merging this.
Working through an issue that seems to have popped up on CI right now:
https://app.circleci.com/pipelines/circleci/2pKxPuEmc4jySZHiGcJrJE/LTM5bKmo57v3FcPSxgh5mk/2/workflows/e26029fa-ab66-40d3-870d-18d59c721a59/jobs/19/parallel-runs/0/steps/0-107?invite=true#step-107-54412_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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "isaacreath (via GitHub)" <gi...@apache.org>.
isaacreath commented on PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#issuecomment-2539837325
Thank you everyone for your review!
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "isaacreath (via GitHub)" <gi...@apache.org>.
isaacreath commented on code in PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#discussion_r1878525869
##########
build.gradle:
##########
@@ -102,18 +105,35 @@ application {
// Take the application out once we're running via Cassandra
mainClassName = "org.apache.cassandra.sidecar.CassandraSidecarDaemon"
applicationName = 'cassandra-sidecar'
- // Config file location should be in file:/// format for local files,
- def confFile = "file:" + File.separator + File.separator + "APP_HOME_TO_REPLACE/conf/sidecar.yaml"
- applicationDefaultJvmArgs = ["-Dsidecar.logdir=./logs",
- "-Dsidecar.config=" + confFile,
- "-Dlogback.configurationFile=./conf/logback.xml",
- "-Dvertx.logger-delegate-factory-class-name=io.vertx.core.logging.SLF4JLogDelegateFactory",
- "-javaagent:APP_HOME_TO_REPLACE/agents/jolokia-jvm-1.6.0-agent.jar=port=7777,host=localhost"]
+ applicationDefaultJvmArgs = ["PLACEHOLDER"]
Review Comment:
When the `startScripts` Gradle task runs, it will create a variable in the start script called `DEFAULT_JVM_OPTIONS` which is set to the values in `applicaitonDefaultJvmArgs`. This variable is purposefully created in a way such that the environment variables that we want to embed into the `DEFAULT_JVM_OPTIONS` will not be evaluated.
The way around this is to generate `DEFAULT_JVM_OPTIONS='PLACEHOLDER'` in the start script, then modify that value after the start script is generated and replace `'PLACEHOLDER'` with the actual default JVM options.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#discussion_r1875013239
##########
build.gradle:
##########
@@ -173,14 +193,22 @@ tasks.named('idea').configure {
// Lets copy the distributions from build/install directory to /bin and /lib
// directories to be aligned with C* distribution format
-tasks.register('copyDist', Copy) {
- from "$buildDir/install/$applicationName"
- into "$projectDir"
+tasks.register('copyDist') {
Review Comment:
we no longer have a `copyDist` task, so this can be removed.
##########
build.gradle:
##########
@@ -38,11 +38,11 @@ plugins {
id 'application'
// since we're using a specific version here, we delay applying the plugin till the all projects
- id "com.github.spotbugs" version "3.0.0" apply false
+ id "com.github.spotbugs" version "5.0.14" apply false
Review Comment:
when I tried upgrading gradle, I was able to get to version `6.20.0` for spotbugs, but I needed a low number of changes on `checkstyle.xml`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c merged PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#discussion_r1880669992
##########
build.gradle:
##########
@@ -38,11 +38,11 @@ plugins {
id 'application'
// since we're using a specific version here, we delay applying the plugin till the all projects
- id "com.github.spotbugs" version "3.0.0" apply false
+ id "com.github.spotbugs" version "5.0.14" apply false
Review Comment:
yes `"6.0.20"` is what I meant. I think it's fine to move to `5.0.14`, so nothing actionable here
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#discussion_r1881107973
##########
gradle/wrapper/gradle-wrapper.properties:
##########
@@ -1,6 +1,7 @@
-#Fri May 01 11:14:42 PDT 2020
-distributionUrl=https\://services.gradle.org/distributions/gradle-6.2.1-all.zip
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
-zipStorePath=wrapper/dists
+distributionUrl=https\://services.gradle.org/distributions/gradle-8.9-bin.zip
Review Comment:
we might also want to downgrade the jar for these to be in sync and not sure if the `gradlew` needs downgrading as well.
##########
build.gradle:
##########
@@ -102,18 +97,35 @@ application {
// Take the application out once we're running via Cassandra
mainClassName = "org.apache.cassandra.sidecar.CassandraSidecarDaemon"
applicationName = 'cassandra-sidecar'
- // Config file location should be in file:/// format for local files,
- def confFile = "file:" + File.separator + File.separator + "APP_HOME_TO_REPLACE/conf/sidecar.yaml"
- applicationDefaultJvmArgs = ["-Dsidecar.logdir=./logs",
- "-Dsidecar.config=" + confFile,
- "-Dlogback.configurationFile=./conf/logback.xml",
- "-Dvertx.logger-delegate-factory-class-name=io.vertx.core.logging.SLF4JLogDelegateFactory",
- "-javaagent:APP_HOME_TO_REPLACE/agents/jolokia-jvm-1.6.0-agent.jar=port=7777,host=localhost"]
+ applicationDefaultJvmArgs = ["PLACEHOLDER"]
}
startScripts {
doLast {
- unixScript.text = unixScript.text.replace("APP_HOME_TO_REPLACE", "\${APP_HOME}")
+ // Config file location should be in file:/// format for local files,
+ def confFile = "file:" + File.separator + File.separator + "\${APP_HOME}/conf/sidecar.yaml"
+
+ // As of Gradle version 7.2, the generated bin/cassandra-sidecar file by default is configured to not allow
+ // shell fragments. Thus, variables like APP_HOME are treated as literals, which prevents us from being able to
+ // start up cassandra-sidecar. See this comment on bin/cassandra-sidecar (line 208):
+ //
+ // # Collect all arguments for the java command:
+ // # * DEFAULT_JVM_OPTS, JAVA_OPTS, JAVA_OPTS, and optsEnvironmentVar are not allowed to contain shell fragments,
+ // # and any embedded shellness will be escaped.
+ // # * For example: A user cannot expect ${Hostname} to be expanded, as it is an environment variable and will be
+ // # treated as '${Hostname}' itself on the command line.
+ //
+ // Replacing the placeholder after the script has been generated will preserve the semantics from Gradle < 7.2.
+ // See this Gradle issue for more context: https://github.com/gradle/gradle/issues/18170.
+
+ def defaultJvmOpts = ["-Dsidecar.logdir=./logs",
+ "-Dsidecar.config=" + confFile,
+ "-Dlogback.configurationFile=./conf/logback.xml",
+ "-Dvertx.logger-delegate-factory-class-name=io.vertx.core.logging.SLF4JLogDelegateFactory",
+ "-javaagent:\${APP_HOME}/agents/jolokia-jvm-1.6.0-agent.jar=port=7777,host=localhost"]
+ unixScript.text = unixScript.text.replace("DEFAULT_JVM_OPTS='\"PLACEHOLDER\"'",
Review Comment:
verified that this is generating something that looks like this:
```
DEFAULT_JVM_OPTS='"-Dsidecar.logdir=./logs" "-Dsidecar.config=file://${APP_HOME}/conf/sidecar.yaml" "-Dlogback.configurationFile=./conf/logback.xml" "-Dvertx.logger-delegate-factory-class-name=io.vertx.core.logging.SLF4JLogDelegateFactory" "-javaagent:${APP_HOME}/agents/jolokia-jvm-1.6.0-agent.jar=port=7777,host=localhost"'
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSSIDECAR-167] Update Gradle Version [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on PR #155:
URL: https://github.com/apache/cassandra-sidecar/pull/155#issuecomment-2518727405
Thanks for the PR @isaacreath!
The Cassandra project does not support Windows, so I think we should exclude the bat file from the repo. What are your thoughts on that?
I will take a look at the PR sometime this week
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org