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

Add log messages in initialization phase #1362

Merged
merged 2 commits into from
Feb 2, 2022

Conversation

yotamN
Copy link
Contributor

@yotamN yotamN commented Feb 2, 2022

I've had to resort to some print debugging because of a bug that occurred only outside Intellij, I've added a few logs that I think could be useful to others as well.

One thing to note: I don't think I saw the loaded input files message even in debug mode, not sure if it's a bug with the verbose flag

@skylot
Copy link
Owner

skylot commented Feb 2, 2022

I don't think I saw the loaded input files message even in debug mode, not sure if it's a bug with the verbose flag

Oh, this is actually a bug, looks like I messed up at

if (logLevel != LogLevelEnum.QUIET) {

here should be

if (logLevel == LogLevelEnum.PROGRESS) {

And, after that change, we can see logs from JadxDecompiler class in verbose:

INFO  - loading ...
DEBUG - Loading plugin: java-convert
DEBUG - Loading plugin: smali-input
DEBUG - Loading plugin: dex-input
DEBUG - Loading plugin: java-input
INFO  - Resolved 3 plugins
DEBUG - Resolved plugins: [dex-input, java-convert, smali-input]
DEBUG - Loading dex: classes2.dex
DEBUG - Loading dex: classes3.dex
DEBUG - Loading dex: classes.dex
DEBUG - Loaded input files with dex-input
INFO  - Loaded 1 inputs

Here are my thoughts:

  • Loading plugin: - is fine
  • Resolved 3 plugins - can be omitted because next line already show that info
  • Loaded input files with dex-input - not really needed because dex-input already printed what was done
  • Loaded 1 inputs - a little confusing, maybe Loaded using 1 inputs plugin will be better (counter for non-empty load results)
  • debug level should be used for all such logs

@yotamN please apply my fix and recheck your changes.
I am fine with more verbose output, but it should be clear and don't contain duplicates :)

I've had to resort to some print debugging because of a bug

What kind of bug? Are you trying to develop your own plugin? I am just curious 😄

@yotamN
Copy link
Contributor Author

yotamN commented Feb 2, 2022

I fixed everything you said and added a fix to show the logs from JadxDecompiler. Although I still think that the message Loaded input files with might be useful because it doesn't assume that the plugin itself have a log message about itself.

What kind of bug? Are you trying to develop your own plugin? I am just curious smile

I'm using Jadx nightly as a library to decompile some APKs so I built it myself and when I worked on it from Intellij it worked great but when I built it myself as an uberjar and run it inside Flatpak it found 0 classes because all the plugins except one were missing. I'm still not sure why the ServiceLoader failed to find it but for now I manually load the plugins.

If I could manage to extract a reproducible example I will open an issue.

@skylot
Copy link
Owner

skylot commented Feb 2, 2022

because all the plugins except one were missing

This can happen if plugin services files were overridden (same file with same path in all plugins).
Shadow jar gradle plugin should be configured like this:

shadowJar {
	mergeServiceFiles()
}

Although I still think that the message Loaded input files with might be useful

Well, you can add it, but in your case this will not help, because it will not appear.

@yotamN
Copy link
Contributor Author

yotamN commented Feb 2, 2022

This can happen if plugin services files were overridden (same file with same path in all plugins).

I will try it, thanks!

Well, you can add it, but in your case this will not help, because it will not appear.

Why will it not appear?

@skylot
Copy link
Owner

skylot commented Feb 2, 2022

Why will it not appear?

Incorrect plugin will return empty ILoadResult so nothing will be added to loadedInputs

@yotamN
Copy link
Contributor Author

yotamN commented Feb 2, 2022

Fair, so I think you can merge it as is if it's okay by you

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

Successfully merging this pull request may close these issues.

2 participants