-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Introduce RISC-V support #48
Conversation
Looks alright, though I won't be able to test it with the hardware I currently have. I might get a RISC-V SBC at some point, though don't see much reason to get one over ARM at this time. I wouldn't worry about 32-bit RISC-V (but worth supporting if it's trivial to). The While I think about it, there's a lot of natives downloaded and bundled into libGDX game JARs (x86, x64, arm32, arm64... potentially RISC-V in future – Windows, macOS, Linux) – maybe an opt-in solution could be realised for that? E.g. if someone is only interested in 64-bit Windows (not unheard of for Steam titles), don't bother with the rest. But this is probably a discussion for elsewhere, and not the most important thing in the world. |
It works🎉
But the gradle build is freaking slow on the board... I guess the jdk is not well optimized for riscv or something. I agree with you about choosing the architecture. Maybe it's something for the gdx-setup app? Or do we add Gradle tasks that generate one jar per architecture? |
I'm not sure, whether this isn't a bit bloated. We already have 6 jars produced with natives, if now desktop gets split up too we end up with like around 23 jars if we split on |
At first glance, we should consider replacing While there, it might also make sense to turn I think this should all be able to do be done while ensuring backwards compatibility too. |
Done, do you think we should deprecate methods with public static final boolean x32 = false;
public static final boolean x64 = true;
public static final boolean ARM = true;
public static final boolean RISCV = true; Should we also change the shared library loader? |
Yeah, the legacy methods should be marked as deprecated.
I was assuming we could replace them with references to the enums in a backwards compatible manner, but I haven't experimented to check if that's actually possible.
I think so, but doing so would be very breaking, and might be worthwhile keeping the old ones as accurate as we can. |
Done, except for the shared library loader. The reason is that I'm not sure where to put the enums because, in the current position, it's not able to access them |
Do you have an idea? Should we move the enum to |
I think move it to SharedLibraryLoader, and make a new static field for the arch. Also leave the old isARM, isRISCV, is64Bit but mark them as deprecated. |
Everything is moved |
public BuildTarget (BuildTarget.TargetOs targetType, boolean is64Bit, String[] cIncludes, String[] cExcludes, | ||
String[] cppIncludes, String[] cppExcludes, String[] headerDirs, String compilerPrefix, String cFlags, String cppFlags, | ||
String linkerFlags) { | ||
public BuildTarget(Os targetType, Architecture.Bitness bitness, String[] cIncludes, String[] cExcludes, String[] cppIncludes, String[] cppExcludes, String[] headerDirs, String compilerPrefix, String cFlags, String cppFlags, String linkerFlags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might make sense to pass here also just the architecture, instead of setting it in newDefaultTarget
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no real opinion on this
gdx-jnigen-loader/src/main/java/com/badlogic/gdx/utils/SharedLibraryLoader.java
Outdated
Show resolved
Hide resolved
gdx-jnigen-loader/src/main/java/com/badlogic/gdx/utils/SharedLibraryLoader.java
Show resolved
Hide resolved
gdx-jnigen-gradle/src/main/java/com/badlogic/gdx/jnigen/gradle/JnigenExtension.java
Show resolved
Hide resolved
Merged! Next step should be PR with |
Not tested yet, love to hear some comments
EDIT: Works flawless