elBukkit / MagicPlugin

A Bukkit plugin for spells, wands and other magic
http://mine.elmakers.com
MIT License
242 stars 148 forks source link

Building Magic from source: Question about the Errorprone IntellIJ Plugin. #1357

Closed Remodactyl closed 3 months ago

Remodactyl commented 3 months ago

I've seen a group of people attempting to build Magic from source using the project cloned from Github, and using the IntellIJ IDEA I see most of them running into the same problem.

image

This flag is part of the Errorprone plugin which presumably you use in developing Magic. And the definition thereof can be found in the Magic-parent Pom.xml file.

image

However, it seems the current version of this plugin, available on IntelIJ, does not resolve this 'Invalid Flag' issue. (even if the Java-compiler is, as per instruction, set to 'javac with error-prone'). My theory is currently that you use an Outdated version of this plugin in the pom.xml file, and that we can no longer find the correct version that recognizes these flags, but this is difficult to test as very little information seems to be openly available about the plugin.

My question is this: If you update your version of the Errorprone plugin, can you still get it to work with your IDEA and Magic in IntellIJ? And if not, considering how many flags are used in the config to suppress warnings, is there still reason for keeping this plugin around?

NathanWolf commented 3 months ago

This is a little weird since the pom file and Maven are supposed to determine what version of errorprone gets used, I am not sure how others are getting different versions. They are all still available since they come from Maven Central.

Anyway I updated errorprone to latest, removed that flag and fixed the errors that popped up. Hopefully that helps!

Remodactyl commented 3 months ago

I see a commit has come in removing the top XepDisableAllChecks flag and updating the Errorprone version, now the warning simply happens for the next flag:

image

This likely means that for some reason on our own builds (mine and that of other developers who have tried building from source) the build is not properly running with errorprone, and the flags are not being defined by the plugin. For some reason, your setup seems to recognize the plugin, do you have any idea what changes you made back when you first started using it to allow for it to work?

NathanWolf commented 3 months ago

I wasn't actually the one to add ErrorProne. As far as I know I didn't need to do anything to my build to make it work. The pom should be all that is needed to specify how the build should work.

Are you just doing like mvn clean package to build and that's it?

NathanWolf commented 3 months ago

Also worth noting that the build server has never had any issues and it's just using a stock JDK.

Remodactyl commented 3 months ago

Uhm, I was running clean and install, let me try with package!

NathanWolf commented 3 months ago

install should be fine, too- that's the same as package except it installs the results to your local maven repo.

What JDK are you using? I think I am using Oracle (21) in both places.

Remodactyl commented 3 months ago

Nope, mvn clean package still gives the same flag errors, currently running JDK 21 (From Oracle)

Remodactyl commented 3 months ago

From how I understand it, Errorprone is essentially just a plugin that helps detect code smells, if you've never noticed this plugin affecting your work, it may be easier to remove it altogether?

NathanWolf commented 3 months ago

I really don't know .... could be a platform difference I guess? I test on Ubuntu and MacOS ... maybe something Windows specific, assuming y'all are on Windows?

If you remove all of the -Xep compiler flags does it work?

NathanWolf commented 3 months ago

Errorprone is super helpful. In fact that two things I had to fix when removing the "DisableAllChecks" flag were real issues that had gone unnoticed. So I'm not really in favor of removing it, I'd prefer to figure out what the actual problem is.

Remodactyl commented 3 months ago

Yup, removing all the arguments in the same clause under errorprone like so: image

works for me. Also I am indeed on Windows, do you use IntellIJ IDEA?

Remodactyl commented 3 months ago

I am also not opposed to figuring this out through a voicecall in the discord if that makes things easier, it's possible this is a difference in our IDEA configurations

NathanWolf commented 3 months ago

I develop and build locally with intellij, but the build server is just straight JDK via Jenkins. I consider it a good secondary check to rule out anything IDE or Mac-specific.

I'm guessing this has something to do with the Windows JDK for whatever reason. I don't see anything via Google though.

I'd be ok with removing most of those flags, but definitely want to keep Xep:RemoveUnusedImports:ERROR .. can you try with just that one? It makes sure the code stays clean, catches unused imports constantly.

It sounds somewhat like errorprone on Windows is just broken, though, and I'm not really sure what I can do about that.

Remodactyl commented 3 months ago

Unfortunately it does not recognize the flag, looks like for the time being windows users will have to know to edit these out if they want to build source code, could be a good temporary solution to mention this in the magic documentation?

Beyond the flag we now just run into the expected: image

'Caused by: org.eclipse.aether.resolution.DependencyResolutionException: The following artifacts could not be resolved: com.elmakers.mine.bukkit:EffectLib:jar:10.3 (absent): com.elmakers.mine.bukkit:EffectLib:jar:10.3 was not found in https://maven.elmakers.com/repository/ during a previous attempt. This failure was cached in the local repository and resolution is not reattempted until the update interval of elMakers has elapsed or updates are forced at org.eclipse.aether.internal.impl.DefaultRepositorySystem.resolveDependencies (DefaultRepositorySystem.java:365)'

Which I assume has to do with EffectLib being modified since this only started about a week ago.

NathanWolf commented 3 months ago

Oops, yeah the EffectLib it just a new version I need to upload to Maven Central.

It should be there now.

As for errorprone, I don't know what else to look at there. @killme are you still around? (if so, hi, long time!) - do you still build Magic, and if so is it on Windows?

The OS is the only variable I can really think of at this point.

Remodactyl commented 3 months ago

Interestingly, I've just tested errorprone on one of my own projects, I copied the maven configuration arguments that magic-parent uses and in my project errorprone DOES WORK! So there must be some difference in the setup for the magic plugin. I am going to keep looking!

Remodactyl commented 3 months ago

Scratch that, I didn't use all the same flags, but it is catching code smells correctly so the plugin is at least partially functional, I'll be curious to see your friend attempt this on Windows in IntellIJ though!

Remodactyl commented 3 months ago

Ok, the same flags are broken in that project for me too, false alarm. Could still be a Windows issue.

NathanWolf commented 3 months ago

Well for now I have commented out the flags. I'd like to put them back eventually, but there are probably better ways (like the errorprone config) of enabling those checks. I have to admit I'm not super well versed in this plugin, but I do like the results.

Remodactyl commented 3 months ago

Yea, it'd be a shame to throw it out, here's hoping your friend is more familiar with it, until then we'll have to watch our own unused import clauses xS

killme commented 3 months ago

I just tried building the latest commit on master, with the commenting reversed and it works fine when building from the commandline. I've always been using Linux and commandline maven / Eclipse with no issues.

Could you try reproducing it by running mvn explicitly. So we can see what it does without the IDE integration. If that still breaks, please use -X to run maven in debug mode to see the full output.

It could be the case that something funky is going on with the line endings on windows, as the \ is used to escape them so the entire argument is in fact kept as a single argument from javac's point of view. Are you using git's autolf by any chance? You could try turning that off using git config core.autocrlf input, and then reverting all local changes to the repository, before trying another build.

NathanWolf commented 3 months ago

It could be the case that something funky is going on with the line endings on windows

Oh that's a good point. One easy thing we could try then is putting the whole command line on one line in the pom.

@Remodactyl can you please edit the pom like this and try it?

                        <arg>-XDcompilePolicy=simple</arg>
                        <arg>
                            -Xplugin:ErrorProne -Xep:ProtectedMembersInFinalClass:ERROR -Xep:RemoveUnusedImports:ERROR -Xep:InlineMeSuggester:OFF -Xep:CatchAndPrintStackTrace:OFF
                        </arg>
Remodactyl commented 3 months ago

Looks like that works! So the issue all this time was that the flags were not being seen as part of the same arg as Xplugin:Errorprone? I had tried deleting the backslashes before but I hadn't moved the errorprone args to the same line yet, and this appears to have fixed it finally. Looks like you have one more commit to make and people will have the errorprone plugin functional. Thanks a ton!

Remodactyl commented 3 months ago

This means you can probably put back the flag you deleted before when we were still testing it. the '-XepDisableAllChecks', since that used to be there before. After that I think this issue is resolved!

NathanWolf commented 3 months ago

Thanks, I just pushed the change.

I was also thinking about putting that DisableAllChecks flag back in, but since the build is building ok with all checks enabled I think I'll just leave it.

It's not totally clear to me if the following flags are needed (maybe :ERROR is the default if you're not disabling checks up front?) but I think I am ok leaving this all as-is for now.