some improvements... optional 😉

[bbortt]

hi there! huge thank you for open sourcing all your code. I've recently installed my own server on a raspberry pi for private usage. it works like a charm 💌

being a developer myself, I've some remarks on the project. mostly convenience things. I leave it up to you if you merge it all, cherry-pick single commits or don't care at all. this PR just is my way to say "thank you".

• added a Maven wrapper for convenience. not everyone has Maven installed on the local machine (e.g. I don't).
• moved into a monorepo with Maven modules. that makes it possible to conquer all builds simultaneously using ./mvnw package. if you still want to build single packages, execute ./mvnw package -pl :your-package (as beforehand). note that this might need adaptions to the deploy.yml job, I am not entirely sure.
• updated some dependencies, most notably resolved CVE-2024-7254 being present in com.mysql:mysql-connector-j:8.3.0. I preferrably don't want vulnerabilities in production code.

more to come. if you wish so 😉

[neon-dev]

Hi and thank you for your suggestions. At the moment I only have a few notes and one change request, but more may follow:

not everyone has Maven installed on the local machine

@Estrayl and I haven't decided whether to add the Maven wrapper yet. Personally, I think it's unnecessary, since all the major Java IDEs ship with Maven and I see no reason to support more exotic development environments.

that makes it possible to conquer all builds simultaneously

Do you mean the servers will build concurrently? Because the parent pom.xml already allows to build everything in one go as it is. Although this is not a feature we use. During development, the IDE takes care of tracking and updating changed classes, and for deployment we do individual builds, as you rarely need to update more than one server.
I'll test and see what practical benefits your changes bring when I find the time, as I haven't really done much with Maven modules yet.

I also have yet to test nightly.yml. Hopefully it has some kind of notification mechanism. If the reports have to be opened manually it is something that can go unnoticed for quite a while. We already have CodeQL, which notifies about vulnerabilities in dependencies, but it doesn't seem to be smart enough to detect them from transitive dependencies.

more to come. if you wish so 😉

Thanks again. We're always happy when someone decides to contribute instead of keeping improvements and fixes to themselves (which is very common in the Aion community 😄).
If you are unsure about future contributions, you can always discuss them beforehand on our Discord.

[bbortt]

thanks for the remarks @neon-dev. I've cherry-pick'ed the dependency update commit for now. let me know if you decide to go for the wrapper and/or other changes as well. I personally don't see Maven being shipped with Eclipse Temurin, but I'll gladly be wrong with that one. the advantage of the wrapper is for sure that "works on my machine" does no longer exist - or not because of Maven.

regarding the nightly job; no, there is no notification in place. an alternative for that would be adding an OSSHR upload & report action. those exist. I preferred the manual check up until now.

edit: oh, sorry. you spoke about IDE's, not distributions. lol

[bbortt]

Do you mean the servers will build concurrently? Because the parent pom.xml already allows to build everything in one go as it is. Although this is not a feature we use. During development, the IDE takes care of tracking and updating changed classes, and for deployment we do individual builds, as you rarely need to update more than one server.
I'll test and see what practical benefits your changes bring when I find the time, as I haven't really done much with Maven modules yet.

ok, I do see that, "one command to rule them all" is not really an addon. but, using the parent for sure has the effect that e.g. the java version must not be declared multiple times. dependencies (-versions) can be managed in one place (e.g. commons module).

individual builds are still possible.

let me know what you think of it, once you tried it. I've now created my "personal cherries" branch, so the PR is more focused on single changes.

[bbortt]

this should do

[sky1683589933]

Hi, may I ask if this can solve the issue of inconsistent versions of "slf4j - API" in dependencies?

[neon-dev]

Hi, may I ask if this can solve the issue of inconsistent versions of "slf4j - API" in dependencies?

Well, there is now one less conflict (one instead of two):

It's not really a problem, the newer version was and still is selected for all the builds. You could define an explicit dependency on it, but with slf4j-api in particular, it's not something to worry about unless there's a vulnerability in the imported version.

[sky1683589933]

It's not really a problem, the newer version was and still is selected for all the builds. You could define an explicit dependency on it, but with slf4j-api in particular, it's not something to worry about unless there's a vulnerability in the imported version.

Yes, I saw the PR regarding this issue and would like to ask by the way.

[neon-dev]

let me know if you decide to go for the wrapper and/or other changes as well.

Hey, sorry for the delay. @Estrayl and I have decided against the Maven wrapper, but the changes to the dependency management and global compiler parameters seem useful.

Just a few last things before I can finally merge your pull request:

1. Please set the properties in the parent pom.xml as follows:

   <properties>
   	<!-- Global -->
   	<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
   	<maven.compiler.release>21</maven.compiler.release>
   </properties>

   The release property is the recommended way to specify the target version since Java 9.
2. Feel free to remove the showWarnings and debug compiler configs from all pom.xmls, as they are already true by default.
3. We prefer 4.8-SNAPSHOT over 4.8.0-SNAPSHOT for this project.
4. I have noticed that mysql-connector-j 8.4.0 still depends on the vulnerable protobuf version, but 9.2.0 fixes this problem and looks like a safe upgrade after initial testing and checking the changelog.

Thanks again!

[bbortt]

@neon-dev I've taken your remarks into consideration. please note that I've moved the management of the plugins into the parent pom.xml. I hope this is to your liking.

one thing regarding

2. Feel free to remove the showWarngings and debug compiler configs from all pom.xml's, as they are already true by default.

please note my comments.

and regarding

I have noticed that mysql-connector-j:8.4.0 still depends on the vulnerable protobuf version, but 9.2.0 fixes this problem and looks like a safe upgrade after initial testing and checking the changelog.

didn't notice it, and so didn't mvnrepository.com. but nice catch ❤️ I did the upgrade, sounds save based on the changelog.

[neon-dev]

During final testing I ran into one problem with this configuration.
I cannot build the servers from their pom.xmls anymore unless I run mvn install on the parent pom.xml once. Otherwise I get

[ERROR] Failed to read artifact descriptor for com.aionemu:commons:jar:4.8-SNAPSHOT
[ERROR] 	Caused by: The following artifacts could not be resolved: com.aionemu:aion-server:pom:4.8-SNAPSHOT (absent): Could not find artifact com.aionemu:aion-server:pom:4.8-SNAPSHOT

Do you know how to solve this without installing the parent pom.xml (+servers) into the local Maven repo?

If there is no other solution, adding this to each server's pom.xmls would at least stop them from being copied into the local Maven repo:

<properties>
	<maven.install.skip>true</maven.install.skip>
</properties>

[bbortt]

I cannot build the servers from their pom.xmls anymore unless I run mvn install on the parent pom.xml once.

can you post the command you're using, please? or do you mean "in IDE" by clicking some "build Maven project" button?

what works is (without previous mvn install): mvn -pl :game-server -am package. that's what the deployment job does, right? nothing is being stored in your local Maven repository.

I could commit this as a run configuration, if wished?

for any IDE integration to work, you'd have to enable resolving of workspace artefacts. I don't not how to do that in other editors, tho. but that requires a previous mvn package -pl :commons -am too.

[neon-dev]

can you post the command you're using, please? or do you mean "in IDE" by clicking some "build Maven project" button?

Classic mvn <phase> from the server directory, which is the same as double-clicking the phase in IntelliJ's Maven sidebar for example (no run config needed).

that's what the deployment job does, right? nothing is being stored in your local Maven repository.

Yes, because it is run with the parent pom.xml. The module syntax is fine for scripts, but I'd still like to retain the ability to run simple maven phases or goals on the individual module's pom.xmls without too many preconditions.

I could commit this as a run configuration, if wished?

Thanks, but I don't think this should be necessary. If this can't be solved inside the pom.xmls in a better way, we should just add the skip property I mentioned earlier to the servers. That way mvn install of the parent pom.xml no longer copies the servers to the local Maven repo, even though they would still be compiled and packaged.

[bbortt]

we should just add the skip property I mentioned earlier to the servers

sure thing; I can do that.

[neon-dev]

Thanks a lot!