triplea-game / triplea

TripleA is a turn based strategy game and board game engine, similar to Axis & Allies or Risk.
https://triplea-game.org/
GNU General Public License v3.0
1.34k stars 393 forks source link

New Install4j builder - could not find root folder error #260

Closed DanVanAtta closed 8 years ago

DanVanAtta commented 8 years ago

Linux or windows, installing using with the new install4j builder has the following error on startup. Does not seem to be more than a warning message about some paths not being available, big world map loaded and played.

Could not find root folder, does not exist:file:/home/dan/triplea_1.8.0.10.520/bin

gaborbernat commented 8 years ago

My initial hunch is that in Version.java we defined versioning as:

return m_major + separator + m_minor + separator + m_point + (noMicro ? "" : (separator + m_micro));

However, now a new number has been added to the end, but not here. Why the new number at the end?

veqryn commented 8 years ago

I thought we agreed to have the last digit be the build digit, and stick with 4 digits overall? (We also agreed that gamesave compatibility would be moved up to the 3rd digit, and I hope there will be a PR to fix that soon, along with updating the docs and message that we spit out to the users when they try to use an incompatible savegame.)

veqryn commented 8 years ago

I do not get this error/warning message, regardless of what getTripleaJarWithEngineVersionStringPath does.

@DanVanAtta do you still get this? and if so, how to recreate?

veqryn commented 8 years ago

Update: I don't get this problem with the all platforms, only with the installed version. The reason being that the installer does not create/put the jar as "triplea.jar" like it should. This is the reason for the error, and it has nothing to do with getTripleaJarWithEngineVersionStringPath.

In fact, the changes made to getTripleaJarWithEngineVersionStringPath would result in this jar thinking it is the old engine jar, instead of thinking it is the new engine jar. So the change that 'fixes' this actually just adds a new bug.

DanVanAtta commented 8 years ago

@veqryn - about build numbers. No attempt has been really to modify the existing structure. We do need to have something that changes whenever a build is done, so appending the travis number to the end is all that was added for now.

DanVanAtta commented 8 years ago

This must be fixed before we can do the next release, red flagging it.

gaborbernat commented 8 years ago

Hello, I have a job interview next Tuesday, hence my hiatus lately, will get back into fixing this next weekend.

gaborbernat commented 8 years ago

I've been looking into this. So aside fixing the error message; specifically about adding or not the JRE.

Here's what I think we should do: make the JRE dynamically optionally bundled. That is the JRE is downloaded at installation start if there are no good Java installed for the user. Still looking if it's possible to also squash the 32 bit and 64 bit versions into one, but seems not at this point.

So that would mean 5 installers:

DanVanAtta commented 8 years ago

" make the JRE dynamically optionally bundled."

That would be sweet. But I'm not sure how to do so unless you have a native script check for a JRE and then act accordingly. I could help write a Mac+Linux shell script that would do that, for windows I think a power shell script would be the way. Overall it does sound like quite the headache initially (but if we have any powershell guru's, then maybe this is a good way to go).

@gaborbernat , wouldn't it be 8 installers, three more the for the non-bundled versioons? According to the source forge download counts, the non-JRe-bundled versions are 9 out of 10 times preferred to the bundled versions.

gaborbernat commented 8 years ago

install4j can check it, it works I already tried this on my branch: https://github.com/gaborbernat/triplea/tree/installer

because the java is downloaded on demand, essentially all packages will be non-bundled :)

DanVanAtta commented 8 years ago

That's good news. Please make it so @gaborbernat !

veqryn commented 8 years ago

cool

DanVanAtta commented 8 years ago

354 created+assigned to @gaborbernat for the dynamic JRE bundle task.

@veqryn , I see the same, when the triplea-{version_number}-all.jar is renamed to triplea.jar, then this goes away.

I'm fairly far along on a fix for this specific error ("unable to find root folder"), assigning to myself.

DanVanAtta commented 8 years ago

Fixed by #362

gaborbernat commented 8 years ago

I find my earlier solution https://github.com/gaborbernat/triplea/commit/029699d900cb8cc11fe80fafaa595936868592d0 better than the fix #362; as it reuses the rename shadow; and puts it into another folder.

Note that your solution full build will generate two triplea.jar into build libs; which makes it somewhat strange. Once the non shadow jar; then the renamed shadow jar. Hence me using:

ext.output = file("$project.buildDir/libs/all/triplea.jar")

gaborbernat commented 8 years ago

Anyways rebased on master my branch. So fixed the merge issues, changed to my solution :)

DanVanAtta commented 8 years ago

362 is not particularly elegant, but it does deliver tangible value in terms of a bug fix. If you have a cleaner implementation, kindly submit a PR.

gaborbernat commented 8 years ago

will do with the final installer implementation together :+1:

DanVanAtta commented 8 years ago

I see, your patch does go a bit further. @gaborbernat Do you know why though you only needed to modify build.install4j in one location, while this PR modified two spots? The archive tag is a second one that this modified.

Testing this, seems fine, but I'll be honest that I changed the config directly and ran gradle, never actually went through the isntall4j UI. Perhaps the second line is not needed?

gaborbernat commented 8 years ago

Actually it's needed. It was a bug on my part not making the second modification. Tried the new installer and works just fine on Linux. Now will work on the optional java part. Will use my Google Drive for testing. And once got it right make the assets pull request.

DanVanAtta commented 8 years ago

@gaborbernat , please note PR #365, recreated your simplification in 029699d