-
Notifications
You must be signed in to change notification settings - Fork 1
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
Minor Improvements #1
base: local-integration
Are you sure you want to change the base?
Minor Improvements #1
Conversation
Submitted by candriver: https://milovana.com/forum/viewtopic.php?p=292660#p292660
This should be a configurable setting.
* Use Gradle packages for most libraries * Restore incorrectly excluded properties file * Deliberately ask for JDK11
Added another small patch. Ought to work, but I would need to find a specifically blocked URL and test, as opposed to just random session triggered ones. |
3122cf5
to
d3d196d
Compare
This also removes a little log spam (As we don't need a stack trace for an exception purely used for flow control)
Tweaked it, so that we now use some custom exceptions to trigger the retry process, as opposed to the generic IO exception. Realistically, I suppose you could get away with a single InvalidImageException but IMHO it's good manners to specify the precise cause, in case we want to handle this better in the future. |
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'm going to try and get a unified gradle script that'll work for both of us. I've cherry-picked that commit from you and then reverted the versions of the many of the pulled in libraries to match those that come with the official TAJ release, otherwise any TeaseAI.jar files that we produce (probably) won't be compatible. I wonder if that's where we're seeing a difference in the need to handle that exception clause; hopefully we can get rid of my cludge so we can both work from the same code base.
I'm also bringing the gradle wrapper up to 6.8.3 to match your version. I didn't realise mine was so old.
It might only be JavaFX that ends up being a bit trickier. I have to use v15 to be compatible with my Linux version.
It wouldn't surprise me either way. Other real possibility is a difference between JDKs somewhere. My gradle alterations pull in a Gradle dedicated copy of OpenJDK11, but I'm presuming you're still using the system provided one, whatever that is. Might also be distro variations at play, but that can quickly get to be a nasty rabbit hole. FWIW, on Windows at least, my built JAR drops quite happily into a working official copy. Not found anything new broken because of it. (n.b. I'll try my branch on a Debian VM at some stage and see what that does) |
I run under OpenJDK 14 by default on Linux Mint which coincides with the statement on the TAJ forum post "You will also need Java 14". It's good that it works under v11 too, I can't really see why it wouldn't. Curiously, now that I've got a fully working gradle build that pulls in all of the dependencies (except EstimAPI and JavaFX) at their original versions, I do have to catch the URISyntax exception again. I thought that was a difference in the signatures between the newer and older versions, but perhaps it's not. That little mystery can be, erm, buried. I'd definitely agree that the API should remain consistent through updates. In the immediate term however, I think it might be best to stick to the original library versions so that there's a better chance of getting that change into the official repo if we can. I'll continue taking a sledgehammer to the various branches that I've got (sorry if you're trying to keep on top of those!), and separate the gradle branch to remove the local changes such as the soft-links. Running under Debian, I'm interested to know if you get something like this when running the JAR: |
36bea35
to
b7db733
Compare
Yeah, definitely doesn't work with that commit on Debian. Oddly, it doesn't work without it either (including the official package):
In order to get it to work, I need to launch the JAR with the OpenJFX module path specifically set, e.g.
It gets the OpenJFX path from this function here: If I understand the JVM specs right, the usr.dir folder is going to resolve to the folder part of JVM binary path, which is /usr/bin/java assuming nothing funky is going on with debian. (although obviously this is a symlink to the 'real' directory deep in /usr/lib/java ) Also wondering if this has been tested on MacOS...... |
Otherwise copying between operating systems may fail
Scratch that, don't copy between Linux and Windows.... |
Copying between Linux and Windows now works with the second platform checking commit. However, if you copy the entire thing and forget to delete the OpenJFX zip file, you're stuck in a loop as it just extracts the thing rather than re-downloading it. |
The only thing I ended up changing was removing the line: I do have a copy of openJFX.zip in the main directory, and it doesn't get stuck in any sort of loop. Historically, there was a problem where the downloaded zip file wasn't a zip file, but was a small text file containing a summary. That was causing a loop where it failed to unpack the zip file and hence couldn't start, but didn't attempt to download again because the zip file was there. The platform check, b402896 , presumably just checks to see if you've got a non-Windows JavaFX installed when running Windows? On a complete side note, I think GodDragon's coding convention is closest to Chromium but with 4 spaces for indentation. It's quite different to my own personal style, and I have to fight it in my own head sometimes 😀. If we want to get some changes in to the master branch, it might be easier for GodDragon to review if the code is closer to his preferred style. That's not a complaint, just an observation! |
Correct. The whole thing really wants patching to see if there's a system JavaFX, but meh. Code styles are a pain in the neck. From what I can see in the codebase and the change history, I wouldn't call it a formal 'convention' as such, perhaps more of an evolved informal style. |
Minor bit of bashing to get this building under Windows. Biggest problem was the missing wrapper properties file, which just seems to be an oversight in the .gitignore
JavaFX could probably be added via Gradle as well, although it's being a little bit of a pain. (tldr: my base JVM / JDK is too old and gradle doesn't change to JDK11 until after the JavaFX task has been triggered)
(commit and PR are both me, I'm just keeping this repo off the main account)