MrTJP / ProjectRed

Redstone Engineering
MIT License
473 stars 183 forks source link

Project Red Compat includes old ComputerCraft API, ignores @API annotations. #728

Closed dan200 closed 9 years ago

dan200 commented 9 years ago

Running the latest Project Red Compat with ComputerCraft 1.73 causes ComputerCraft to crash when CC calls an API method that was introduced after ComputerCraft 1.65. As far as I can tell, this is for two reasons:

1) Project Red Compat includes a copy of the ComputerCraft 1.65 API, despite assurances on this bug tracker that it would stop doing so. This wouldn't be a problem, except:

2) Project:Red's version of the API is being loaded instead of ComputerCrafts newer version, in spite of the presence of @API annotations on both with correct version numbers. I can't figure out how, but it seems something is causing the @API annotations on Project Reds classes to be ignored. I asked a forge dev about this a while ago, and the theory then was the use of core modding may cause this.

dan200 commented 9 years ago

(A stopgap solution to this problem is just to ship the 1.73 API, but that will only postpone the bug until later)

Vexatos commented 9 years ago

I can't figure out how, but it seems something is causing the @API annotations on Project Reds classes to be ignored.

@dan200 They are not. the version part of the annotation just isn't used for anything other than dependency management (i.e. the dependencies part in your @Mod). FML ignores the version number when loading APIs, it will simply load the first one it finds, which, in this case, is apparently P:Red. Do not ask me why FML isn't checking versions and loading the latest API.

FYI: If a coremod is shipping an API in its .jar file, it will indeed be always the one being loaded. But if there is no coremod shipping that API, the case explained above applies.

dan200 commented 9 years ago

If that is the case, then I have been mislead completely on the purpose of the @API annotation. I was first advised to use it by cpw himself :/

Vexatos commented 9 years ago

@API simply is there to check if an API is present and add mod integration based on the API's presence instead of a mod's presence (for example if you want to use the CoFH API without CoFHLib being there). It also allows you to depend on a certain version/version range of an API. That's basically all it does right now.

MrTJP commented 9 years ago

Temporarily updated API with bbb24bb3ec77bfecee4ce4754d0354e633d3d538. Released a patch version. Will continue to hunt for a solution.

Vexatos commented 9 years ago

Oh, another note: If the mod specified as the owner of the @API is present, its API will always be loaded instead of the one found first (at least it should), unless, again, there is a coremod shipping the API, that one always has priority due to it being injected into the classpath much more early.

dan200 commented 9 years ago

Vexatos: It sounds like you've solved the problem. ComputerCraft is the owner of the API, but PR:Compat's version of the API is being loaded instead, because it's a core mod. Does it need to be?

Vexatos commented 9 years ago

If P:Red is a coremod (specifically, if the jar containing the API also contains a coremod), that will ALWAYS be loaded first. Coremods should never be shipping APIs because of the issues it's causing. They shouldn't ever do that and rather use the @Optional annotations for integration.

dan200 commented 9 years ago

Agreed. Let's leave this to @MrTJP to look into.

Vexatos commented 9 years ago

@MrTJP You said in https://github.com/MrTJP/ProjectRed/issues/652#issuecomment-76169538 that you are still getting crashes. If you do, could you please post the crash log(s)? I am pretty experienced with the use of @Optional annotations, so I might be able to help you make P:Red not ship any APIs.

MrTJP commented 9 years ago

It's interesting that you bring this up. I looked into it and realized the compatibility jar's core mod is dormant ever since CC added the redstone APIs, and will be removed ASAP. This will fix the issues?

And @Dan200, sorry for the problems, I really did spend quite a bit of time trying to fix this.

On Feb 26, 2015, at 11:58 AM, Vexatos notifications@github.com wrote:

@MrTJP You said in #652 (comment) that you are still getting crashes. If you do, could you please post the crash log(s)? I am pretty experienced with the use of @Optional annotations, so I might be able to help you make P:Red not ship any APIs.

— Reply to this email directly or view it on GitHub.

Vexatos commented 9 years ago

@MrTJP if you remove the coremod in that particular .jar file and add CC, it should work. Still, you should get rid of as many shipped APIs as possible and use @Optional instead. If it keeps crashing without the API, you simply didn't use enough @Optionals :smile:

dan200 commented 9 years ago

@MrTJP I'm extremely grateful for your help here! My apologies if my tone ever seemed to suggest otherwise. Hooray for fixes!

MrTJP commented 9 years ago

@Vexatos even with the core mod removed, it seems as though FML is completely ignoring the optionals. Here is the full client log if it helps. http://paste.ee/p/HdnqT

Weird though. Don't know where else to stick optionals into. https://github.com/MrTJP/ProjectRed/blob/d8afcfd3a1f738c6427783e7ae4c8d2b8300e779/src/mrtjp/projectred/compatibility/computercraft/PluginCC_BundledCable.scala

Vexatos commented 9 years ago

The client log doesn't contain any reference to any "Optional" thing at all. It should print something in either case (Should log that an optional removal has been skipped as well as an optional thing being removed). That's really weird. As if it never detected the annotation at all.

fnuecke commented 9 years ago

This is an issue with how Java loads classes in general. On line 20 of the PluginCC_BundledCable class there's a method call passing an interface. Java will try to classload that interface, even if the code is never reached. There's some discussion on this behavior over on stackoverflow if you're interested.

One workaround is to add a layer of indirection, i.e. add the init method to a singleton, for example, and call that from the actual init method.

MrTJP commented 9 years ago

Thanks @fnuecke, it worked! Just used a static methods to load the interfaces instead of doing it directly.

Fixed via 1c566e837dcb4632055273843f90facfe85e454f

dan200 commented 9 years ago

hooray!