avoid snapshots being overridden

[oniatus]

Snapshot modules are overridden by other snapshot modules with the same version.
For non-snapshot modules, the logic is "first come first serve" (first part of the modified if-statement).
However for snapshots this turns into "last come first serve" (second statement).

This crashed my terasology instance, where a snapshot jar was loaded instead of my workspace module. Leading to classloading issues which are not visible in the workspace where i developed against my directory module.
See ModulePathScanner#L77 for the default order of module imports.

I think it is wrong to override a registered module if another module with exact the same name and version is found later on, independent if snapshot or not. The latestModules map will handle the latest version (including snapshots) by default.

[immortius]

Hmm.

I think the logic should actually be - add the module if it isn't already present at that version, or if module is non-snapshot and the existing module is a snapshot at that version.

[immortius]

To expand on that I believe that actual released versions should trump snapshot versions. Which is what the code is doing. The only issue is that later snapshots are trumping earlier snapshots, so an extra check should be added for that, rather than removing the ability for released versions to trump snapshot versions.

[Cervator]

Linking to MovingBlocks/Terasology#2223 which I believe is another expression of this situation. @kaen just hit it as well trying to run some unit tests with Pathfinding present in both source and binary form.

Does that necessarily tie into the snapshot bit though? Or is what I'm thinking about separate but similar? Does source vs. binary matter on its own or is that just because source tends to / always counts as snapshot? I forget the exact details from Terasology Gradle land.

In any case it would probably be wise to add some more logging for this case? I think we cover having multiple jars for the same module present in /modules but does that trigger in the case of a source dir + binary?

Also @oniatus you can take out the version bump, we increment to next snapshot version right after we do a release. 5.1.2 isn't out yet :-)

[oniatus]

To expand on that I believe that actual released versions should trump snapshot versions. Which is what the code is doing. The only issue is that later snapshots are trumping earlier snapshots, so an extra check should be added for that, rather than removing the ability for released versions to trump snapshot versions.

The TableModuleRegistryTest.nonSnapshotOverridesShapshot still works with the change.
Module fooBar-1.0.0-SNAPSHOT and fooBar-1.0.0 are two different entries in the registry. The release implicit trumps the snapshot because 1.0.0 > 1.0.0-SNAPSHOT, causing the release version to be in the latestModules.

By thinking about it again, it is not a problem of overriding but logging the error of duplicate modules to the user because we really should not depend on the load order of the environment in the registry.
For this reason, i would keep the change on the add logic (never override a registered module) but log an error in the case if the modules table contains a similar module.

For my workspace (and for the linked issue), the correct solution would be to bump the workspace version.

[immortius]

Oh, I see. I wonder why that check was added then... Probably a mistake from a first attempt at supporting snapshots I guess.

Happy with your PR, with one small request: can you change LOG to logger in line with other uses of Logger in the code base (I follow the SLF4J recommended declaration idiom as described here: http://slf4j.org/faq.html#declaration_pattern ).