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

Provide option to enabled DEBUG level logging when using the CLI #226

Closed
mark-vieira opened this issue Mar 28, 2023 · 12 comments · Fixed by #227
Closed

Provide option to enabled DEBUG level logging when using the CLI #226

mark-vieira opened this issue Mar 28, 2023 · 12 comments · Fixed by #227
Assignees
Milestone

Comments

@mark-vieira
Copy link
Contributor

As discussed in elastic/elasticsearch#94773 it would be helpful to be able to opt into more verbose logging when using the CLI, in addition to the existing option for the build tool plugins.

@uschindler
Copy link
Member

uschindler commented Mar 29, 2023

See the new PR #227.
I am doing some testing, but it restores previous functionality if you start the CLI tool with --debug command line option. There are only slight changes:

  • Prefix of message is "DEBUG".
  • Message comes on stdout, not stderr (as it is now <=INFO). (reverted this, all non-info messages are emitted on stderr).
  • The suffix of message was removed ("please fix classpath").

@uschindler
Copy link
Member

uschindler commented Mar 29, 2023

@uschindler
Copy link
Member

uschindler commented Mar 29, 2023

Hi, I tried my local checkout with a bit of hacking with OpenSearch. Passes after changing the regex and adding --debug to command line options.

If anybody, @mark-vieira or @reta, could have a quick look at the PR #227 for cross-checks or any suggestions?

Here is my patch, including some hacks to make mavenLocal() work for testing:

 buildSrc/build.gradle                                               | 3 ++-
 .../opensearch/gradle/precommit/ThirdPartyAuditPrecommitPlugin.java | 3 ++-
 .../java/org/opensearch/gradle/precommit/ThirdPartyAuditTask.java   | 6 ++----
 buildSrc/src/testKit/thirdPartyAudit/build.gradle                   | 3 ++-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/buildSrc/build.gradle b/buildSrc/build.gradle
index 105444b1e6c..6a5e68930ab 100644
--- a/buildSrc/build.gradle
+++ b/buildSrc/build.gradle
@@ -96,6 +96,7 @@ tasks.withType(JavaCompile).configureEach {
 repositories {
   mavenCentral()
   gradlePluginPortal()
+  mavenLocal()
 }
 
 dependencies {
@@ -114,7 +115,7 @@ dependencies {
   api 'gradle.plugin.com.github.johnrengelman:shadow:7.1.2'
   api 'org.jdom:jdom2:2.0.6.1'
   api "org.jetbrains.kotlin:kotlin-stdlib-jdk8:${props.getProperty('kotlin')}"
-  api 'de.thetaphi:forbiddenapis:3.5'
+  api 'de.thetaphi:forbiddenapis:3.6-SNAPSHOT'
   api 'com.avast.gradle:gradle-docker-compose-plugin:0.16.11'
   api "org.yaml:snakeyaml:${props.getProperty('snakeyaml')}"
   api 'org.apache.maven:maven-model:3.9.1'
diff --git a/buildSrc/src/main/java/org/opensearch/gradle/precommit/ThirdPartyAuditPrecommitPlugin.java b/buildSrc/src/main/java/org/opensearch/gradle/precommit/ThirdPartyAuditPrecommitPlugin.java
index 06269cccd52..7aa3802ace1 100644
--- a/buildSrc/src/main/java/org/opensearch/gradle/precommit/ThirdPartyAuditPrecommitPlugin.java
+++ b/buildSrc/src/main/java/org/opensearch/gradle/precommit/ThirdPartyAuditPrecommitPlugin.java
@@ -51,7 +51,8 @@ public class ThirdPartyAuditPrecommitPlugin extends PrecommitPlugin {
     public TaskProvider<? extends Task> createTask(Project project) {
         project.getPlugins().apply(CompileOnlyResolvePlugin.class);
         project.getConfigurations().create("forbiddenApisCliJar");
-        project.getDependencies().add("forbiddenApisCliJar", "de.thetaphi:forbiddenapis:3.5");
+        project.getRepositories().mavenLocal();
+        project.getDependencies().add("forbiddenApisCliJar", "de.thetaphi:forbiddenapis:3.6-SNAPSHOT");
 
         Configuration jdkJarHellConfig = project.getConfigurations().create(JDK_JAR_HELL_CONFIG_NAME);
         if (BuildParams.isInternal() && project.getPath().equals(":libs:opensearch-core") == false) {
diff --git a/buildSrc/src/main/java/org/opensearch/gradle/precommit/ThirdPartyAuditTask.java b/buildSrc/src/main/java/org/opensearch/gradle/precommit/ThirdPartyAuditTask.java
index 88af1ef8c94..6139291b9be 100644
--- a/buildSrc/src/main/java/org/opensearch/gradle/precommit/ThirdPartyAuditTask.java
+++ b/buildSrc/src/main/java/org/opensearch/gradle/precommit/ThirdPartyAuditTask.java
@@ -79,9 +79,7 @@ import java.util.stream.Stream;
 @CacheableTask
 public class ThirdPartyAuditTask extends DefaultTask {
 
-    private static final Pattern MISSING_CLASS_PATTERN = Pattern.compile(
-        "WARNING: Class '(.*)' cannot be loaded \\(.*\\)\\. Please fix the classpath!"
-    );
+    private static final Pattern MISSING_CLASS_PATTERN = Pattern.compile("DEBUG: Class '(.*)' cannot be loaded \\(.*\\)\\.");
 
     private static final Pattern VIOLATION_PATTERN = Pattern.compile("\\s\\sin ([a-zA-Z0-9$.]+) \\(.*\\)");
     private static final int SIG_KILL_EXIT_VALUE = 137;
@@ -367,7 +365,7 @@ public class ThirdPartyAuditTask extends DefaultTask {
             spec.jvmArgs("-Xmx1g");
             spec.jvmArgs(LoggedExec.shortLivedArgs());
             spec.getMainClass().set("de.thetaphi.forbiddenapis.cli.CliMain");
-            spec.args("-f", getSignatureFile().getAbsolutePath(), "-d", getJarExpandDir(), "--allowmissingclasses");
+            spec.args("-f", getSignatureFile().getAbsolutePath(), "-d", getJarExpandDir(), "--debug", "--allowmissingclasses");
             spec.setErrorOutput(errorOut);
             if (getLogger().isInfoEnabled() == false) {
                 spec.setStandardOutput(new NullOutputStream());
diff --git a/buildSrc/src/testKit/thirdPartyAudit/build.gradle b/buildSrc/src/testKit/thirdPartyAudit/build.gradle
index 33ba77e2bef..90556edda09 100644
--- a/buildSrc/src/testKit/thirdPartyAudit/build.gradle
+++ b/buildSrc/src/testKit/thirdPartyAudit/build.gradle
@@ -37,10 +37,11 @@ repositories {
     }
   }
   mavenCentral()
+  mavenLocal()
 }
 
 dependencies {
-  forbiddenApisCliJar 'de.thetaphi:forbiddenapis:3.5'
+  forbiddenApisCliJar 'de.thetaphi:forbiddenapis:3.6-SNAPSHOT'
   jdkJarHell 'org.opensearch:opensearch-core:current'
   compileOnly "org.${project.properties.compileOnlyGroup}:${project.properties.compileOnlyVersion}"
   implementation "org.${project.properties.compileGroup}:${project.properties.compileVersion}"

@uschindler
Copy link
Member

Does anybody has an idea why the forbiddenapis JAR is mentioned 3 times in dependencies. Most crazy was that it is also part of Java source code!?! @reta

@reta
Copy link

reta commented Mar 29, 2023

@uschindler I do not know the history but here are my guesses:

  • ThirdPartyAuditPrecommitPlugin needs the forbiddenApis so it adds all the time
  • the buildSrc uses forbiddenApis directly (since it cannot use ThirdPartyAuditPrecommitPlugin as it builds it)
  • for buildSrc/src/testKit/thirdPartyAudit/build.gradle, I think it is not needed (should be provided by ThirdPartyAuditPrecommitPlugin)

@uschindler
Copy link
Member

@uschindler I do not know the history but here are my guesses:

  • ThirdPartyAuditPrecommitPlugin needs the forbiddenApis so it adds all the time

That's the craziest to me. I am fine with it, but the version number should not be hardcoded there, maybe it should be a global property (like Lucene uses, it has all helper versions in a map on project.ext.

  • the buildSrc uses forbiddenApis directly (since it cannot use ThirdPartyAuditPrecommitPlugin as it builds it)

This is to execute the standard plugin-provided forbiddenApis (gradlew forbiddenApis).

  • for buildSrc/src/testKit/thirdPartyAudit/build.gradle, I think it is not needed (should be provided by ThirdPartyAuditPrecommitPlugin)

I can try to remove that one, I had the same idea.

@mark-vieira
Copy link
Contributor Author

Does anybody has an idea why the forbiddenapis JAR is mentioned 3 times in dependencies. Most crazy was that it is also part of Java source code!?! @reta

The main two are as @reta mentiones. First the plugin adds it as a default dependency and also by the build logic itself. It could read this from the same place used by the build in theory so this isn't duplicated.

@uschindler
Copy link
Member

Release is coming later today.

@uschindler
Copy link
Member

Release 3.5.1 is on plugin portal, Maven takes a bit longer. I will update the Opensearch PR once its on all repos.

@reta
Copy link

reta commented Mar 30, 2023

Release 3.5.1 is on plugin portal, Maven takes a bit longer. I will update the Opensearch PR once its on all repos.

Thanks a lot @uschindler !

@uschindler
Copy link
Member

@mark-vieira
Copy link
Contributor Author

Thanks @uschindler!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants