search

FAForever / downlords-faf-client

Official client for Forged Alliance Forever
https://faforever.com
MIT License
198 stars 121 forks source link

Reduce project setup friction #946

Closed Katharsas closed 5 years ago

Katharsas commented 6 years ago

Eclipse & JavaFx

Eclipse tries to detect and block access to classes/methods that it deems "non-public" (part of internal code rather than public API). This detection usually works fine but is a known problem for JavaFX. Other methods/classes which it blocks:

This can be solved by explicitly allowing access with gradle eclipse plugin. Gradle Eclipse plugin does not work with the current Gradle version, so a Gradle upgrade would be needed for this.

Java 10

The current gradlew.bat wrapper (NOT Gradle itself; the wrapper which downloads/upgrades/executes gradle) crashes if executed while Java 10 is default on system (which is the case if Java 10 was installed on the system).

Since the Gradle wrapper is responsible for selecting the right JDK when executing Gradle, this can not be fixed by passing the right JDK via args. So, a gradle upgrade would be needed to fix this, too.

Gradle upgrade

Currently investigating.

Pull request will land when done. Can I assign myself to this issue?

micheljung commented 6 years ago

You need to use the JDK 8 for now, jdk 10 upgrade is WIP in one of my branches :)

Katharsas commented 6 years ago

Yes, but you cannot even pass JDK 8 to Gradle to make it use JDK 8. The simple existance of Java 10 (as default) on a system will cause any Gradle commands to fail. Edit: Upgrading to recent gradle fixes this, no changes needed to anything else.

Katharsas commented 6 years ago

Ok, so the old Gradle resolves Hamcrest to 1.4-atlassian-1: https://mvnrepository.com/artifact/org.hamcrest/hamcrest-library/1.4-atlassian-1 Seems to be a weird version, does not hava JavaDoc online, but ok. It seems to have most of the things that are in 2.0,0.0. So new gradle needs to resolve to 1.4 instead of 1.3

Katharsas commented 6 years ago

So, in Gradle version 4.3 a bug was fixed, that would cause Gradle to resolve to dependencies which are too new / have breaking changes (like Hamcrest 1.4 i guess). So the correct behaviour is actually to resolve to 1.3 it seems. This means i will need to edit some tests that use Hamcrest 1.4 functions instead of Hamcrest 1.3 functions. Or use the spring dependencies to force every version of Hamcrest to 1.4. Edit: Only had to change emptyString() Matcher function to isEmptyString(), one occurence.

micheljung commented 6 years ago

Why would we want Hamcrest 1.3 instead of 2.0.0?

Katharsas commented 6 years ago

Well all the other libs (junit, testfx, hamcrest-reflection, spring-boot-starter-test) expect 1.3. So we cannot really use 2.0, because 2.0 does not have certain functions that 1.3 has (some were renamed), so if those libs would call those functions, we would get a "NoSuchMethodError" at runtime (or rather, at startup-classloader-doing-its-thing time).

We didn't get that eror yet, because apparently those libs did not call ths functions yet. But i think that is just luck.

1-alex98 commented 6 years ago

Yeah actually that is a good point.

Katharsas commented 6 years ago

Can i get a review on the PR? Its very little changes anyway.

1-alex98 commented 6 years ago

Yeah reviews take there time

Katharsas commented 5 years ago

Closed due to all of this working in more recent Eclipse version.