Fix: changes to ATs break multi-module projects #940
Conversation
…r us to generate a new artifact in some scenarios when the ATs have been changed. Making the comparison here in MinecraftUserRep::findFile is problematic and not necessary. The HashStore will look for a cache hit/miss in later "find***" calls. The HashStore compares the current AT_HASH with the cached AT_HASH and will return a Cache Miss if the ATs have changed.
…fluke because the mapping is stripped immediately after the athash)
|
TeamCity has failed to build. I had that error locally and fixed it by pinning the version of GradleUtils. Perhaps you have the correct fix at your fingertips?
|
|
@LexManos Happy New Year. Are you able to adopt this change? |
|
The problem is I cant reproduce your issue, and logically your change doesn't make sense. You're somehow requesting an artifact when its not configured to create that artifact. I'm not sure how you're doing that. What this check is doing is verifying that the plugin knows how to make the artifact you're requesting. Your change is similar to saying "I know they requested MC 1.12 but lets just generate for 1.20 instead" Its a known 'qwerk' of FG3-6 that you have to import a project twice to get the correct sources to line up. If thats what you're running into then refreshing the project twice should solve it. If its not then i'd need to see your setup and find a way to reproduce your issue. |
|
Thanks for this. I didn't know about the double-import qwerk. And will try it. I have imported so many times I'd be surprised if that fixes the issue, but I probably always take some action between imports. So... maybe. I believe the check that I have proposed for removal is simply checking to see if the name of the artifact matches the name of the artifact last time it was generated. However, it is being fired too early as the check to see what artifacts this plugin can actually make has not yet been executed. If the name does not match, the code assumes generating the artifact is not possible and quits. I think the check is more like "you asked for an artifact called XYZ but the last one I made was called ABC so I'm not going to try to make XYZ even though I might know how." There is a second mechanism, included in the code, to check whether the plugin is able to provide the requested artifact and it does a better job as it is in a better position in the code. It checks whether the artifact has actually been previously made and is in the file system, rather than passing/failing a check on the string from the filename alone. This mechanism would not allow the plugin to serve 1.20 if 1.12 was requested. The only check I have removed is the comparison of the AT Hash. Version or mappings comparison remains unchanged. Although, I propose they could be removed from findFile() as requesting a differing version would cause a Cache Miss in findPom() [This is complete untested speculation] While the following test is far from conclusive, I'd be interested in the answer: Does the change in this PR cause any of your (test?) projects to fail to build correctly? Thanks again for your consideration on this. |
No, that is not what you're change is doing, it is removing the check if the plugin knows how to generate the expected artifact.
What check are you referring to here? All configuration is done before the artifacts are resolved.
That is explicitly wrong. The repo only knows how to generate one artifact. And only with one specific AT configuration. Whichever one that would be based on user config. There is no possible way that the repo would know how to generate an AT with a different hash so there is no "i might know". I think you're fundamentally misunderstanding what's going on.
What mechanism? If you're referring to the Cacheing system, thats not correct at all.
This is correct, FG is designed to provide one artifact at a time. It can not supply 1.20 if 1.12 is requested because it doesn't know how to build 1.20. That is explicitly why there explicitly a check for one
It would cause a lot more then a cache miss, it would cause the fundamental breaking of assumptions FG is built on.
Yes, but it's highly dependent on how Gradle is feeling at that time. The entire reason the AT hash was put into place was because of Gradle's caching system. The AT Hash is a cache buster system. So building a project, modifying AT and then using it in your code, then trying to build, would result in a access issue depending on how aggressive Gradle's caching system wanted to be. I dont think that this is a viable PR as the system fundamentally doesn't work how you think it does. |
I'm working on a multi-module Minecraft Forge project. We were originally using a build script that compiled the dependency-modules into Jars and then had the master module depend on the Jars. I upgraded the buildscript to work as a true Gradle Multi-module project but, since then, when pulling commits that included changes to AccessTransformers, IDEA would complain about not being able to find the sources.
I think I have stumbled on the solution and would be very pleased and grateful if you could merge this PR.
Quitting early if
AT_HASHdoes not matchathashremoves the chance for us to generate a new artifact in some scenarios when the ATs have been changed.Making the comparison here in
MinecraftUserRep::findFileis problematic and not necessary. TheHashStorewill look for a cache hit/miss in laterfind***calls. TheHashStorecompares the currentAT_HASHwith the cachedAT_HASHand will return a Cache Miss if the ATs have changed.