Skip to content
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

Directly upload Mac / Windows executables. #93

Merged
merged 27 commits into from Oct 9, 2021

Conversation

heeheehee-kolmafia
Copy link
Contributor

Currently, we're generating macOS.zip, Windows.zip which each only contain a single file. We can upload them directly in each release.

heeheehee-kolmafia and others added 21 commits October 4, 2021 22:58
This creates a binary in build/KoLmafia/bin/KoLmafia.

This change also adds a pruneBin task which is always run. I tried to
figure out how to set up inputs / outputs, but jpackage insists on
creating the output directory itself and will generate an error if
said directory already exists.

Follow-up changes will involve tinkering with a build matrix and
looking at the resulting artifacts.
I'm not actually sure what the resulting files will look like on
Windows / Mac, so here's to hoping I can actually see the artifacts
before trying to publish them.
This will allow for more experimentation without pushing to main.
Hopefully Mac artifacts end in dmg, and Windows artifacts end in
exe. But maybe not...
Apparently Mac didn't create build/KoLmafia/bin.
Still not seeing the output of Mac builds, although I now see:

- Linux: ${dist}/KoLmafia/bin/KoLmafia
- Windows: ${dist}/KoLmafia/KoLmafia.exe

Perhaps if I'm lucky, Mac is directly in ${dist}/KoLmafia.dmg?

If I'm unlucky, it won't be present at all. :(
This caused a nasty bit of infinite recursion. Whoops.

Let's instead move things into build/bin.
This allows us to get all of the artifacts in one single container so
we can theoretically publish them all at once later.
We actually need everything in the generated directory, not just the
runner binary.
This attempted to save maybe one level of directory nesting, which I
don't think actually matters in practice.
This splits the existing daily job into two targets: jar and
release. `release` in turn depends on both `jar` and `bin`.

'bin' generates files Windows.zip, Linux.zip, and macOS.zip.
It doesn't feel appreciably different, but maybe it's more readable.
Mac creates the directory `build/releases/KoLmafia.app`, whereas Linux
/ Windows create `build/releases/KoLmafia`. jpackage will create all
prerequisite directories, so it's fine with either deleting the inner
directory, or its parent.
This uses the existing martini glass that was the KoLmelion icon once
upon a time.
Currently, we're generating macOS.zip, Windows.zip which each only
contain a single file.

This change also cleans up jarbundler / jsmooth utils now that we're
using + relying on jpackage for creating platform-specific builds.

We also reuse the Windows icon for Linux (and rename it to get rid of
the legacy KoLmelion name), which is copied into util/linux to preempt
confusion along the lines of "why are we using this windows/ file in
the linux build".
@heeheehee-kolmafia heeheehee-kolmafia requested a review from a team as a code owner October 6, 2021 12:39
gausie
gausie previously approved these changes Oct 6, 2021
Copy link
Contributor

@gausie gausie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. We should be able to make an .AppImage for the linux release too though

@@ -105,6 +109,8 @@ jobs:
files: |
dist/*.jar
releases/*.zip
releases/*.exe
releases/*.pkg
tag_name: r${{ env.KOLMAFIA_VERSION }}
name: ${{ env.KOLMAFIA_VERSION }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add an "r" prefix here too? Or even "Revision " or "Release "

build.gradle Show resolved Hide resolved
@gausie
Copy link
Contributor

gausie commented Oct 6, 2021

Ah I see what's going on with Linux. Look at https://bugs.openjdk.java.net/browse/JDK-8225428 - "APP_IMAGE" doesnt exist, so we're actually just seeing fallthrough behaviour. Does that mean "deb" works?

build.gradle Outdated
@@ -265,14 +265,15 @@ tasks.jpackage {
appVersion = new Date().format("yy.MM")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to use our rev number here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It throws an error if it's not in the approved format:

> Task :jpackage FAILED Custom actions are attached to task ':jpackage'. Caching disabled for task ':jpackage' because: Caching has not been enabled for the task Task ':jpackage' is not up-to-date because: Task has not declared any outputs despite executing actions. Looking for jpackage in toolchain Toolchain is not configured Getting jpackage from java.home Using: /Library/Java/JavaVirtualMachines/zulu-16.jdk/Contents/Home/bin/jpackage --type pkg --app-version 23445-M --main-class net.sourceforge.kolmafia.KoLmafia --main-jar KoLmafia-25750-M.jar --dest /Users/mcroft/projects/KM-HeeHeeHee/build/releases --icon /Users/mcroft/projects/KM-HeeHeeHee/util/macosx/limeglass.icns --input /Users/mcroft/projects/KM-HeeHeeHee/dist jpackage output:
Bundler Mac PKG Package skipped because of a configuration problem: "Version [23445-M] contains invalid component [23445-M]"
Advice to fix: Set a compatible 'appVersion' or set a 'mac.CFBundleVersion'. Valid versions are one to three integers separated by dots.

`:jpackage (Thread[Execution worker for ':',5,main]) completed. Took 0.184 secs.
Closing Git repo: /Users/mcroft/projects/KM-HeeHeeHee/.git

FAILURE: Build failed with an exception.
`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we instead use yy.MM.dd? Or 'yy.MM.' + version or something that gives us finer granularity?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could. We’d need to see how it worked. this is not the revision and our revisions don’t work. I arbitrarily picked the same scheme we used before.

I think it puts that in the installer name, but not the installed app.

@gausie
Copy link
Contributor

gausie commented Oct 6, 2021

Ah I see what's going on with Linux. Look at https://bugs.openjdk.java.net/browse/JDK-8225428 - "APP_IMAGE" doesnt exist, so we're actually just seeing fallthrough behaviour. Does that mean "deb" works?

I can confirm that

	linux {
		type = 'deb'
		icon = 'util/linux/KoLmafia.ico'
	}

actually works. Maybe we should consider only aiming at Debian? The AppImage thing just seems broken.

@heeheehee-kolmafia
Copy link
Contributor Author

APP_IMAGE works for me locally, as well as when I download and unzip the latest release. I just have to manually run ./KoLmafia/bin/KoLmafia. It's not a real app-image....

Using deb means that we're foregoing support for Fedora, RHEL, CentOS (all rpm-based), and basically everyone else who's not on a deb-based distro.

@gausie
Copy link
Contributor

gausie commented Oct 6, 2021

APP_IMAGE works for me locally, as well as when I download and unzip the latest release. I just have to manually run ./KoLmafia/bin/KoLmafia. It's not a real app-image....

Using deb means that we're foregoing support for Fedora, RHEL, CentOS (all rpm-based), and basically everyone else who's not on a deb-based distro.

Try running with any other text instead of APP_IMAGE and you'll have the same result.

We can also do rpm as a target if necessary.

@heeheehee-kolmafia
Copy link
Contributor Author

Yeah, I saw. The problem is that if I run with deb or rpm without having the corresponding tools installed, jpackage fails entirely.

@BadHorseMonkey
Copy link
Contributor

@fewyn and I figured out the issue with windows (by default it puts things in C:\Program Files, by default it wants to write config files to the Current Working Directory if it doesn't find them, by default Windows says Hell No when someone wants to write to C:\Program Files and that said the JDK failed to launch.

Otherwise it works perfectly. :/
We added the dir chooser option and maybe also a default directory, but users may need to be steered towards using something like %CSIDL_PROFILE%\KoLmafia

And I'm about to change and test the mac DMG solution, expect changes incoming...

@BadHorseMonkey
Copy link
Contributor

BadHorseMonkey commented Oct 6, 2021

Yeah, I saw. The problem is that if I run with deb or rpm without having the corresponding tools installed, jpackage fails entirely.

Have you considered setting an environment variable or whatever the gradle equivalent of an ANT property is to something like package.UnixType = (DebToolsInstalled) ? 'deb' : 'APP_IMAGE?
Then all we have to do is figure out how to test for our tools...

Environment property, at worst.

# Conflicts:
#	.github/workflows/build.yml
#	build.gradle
@heeheehee-kolmafia
Copy link
Contributor Author

Yeah, I saw. The problem is that if I run with deb or rpm without having the corresponding tools installed, jpackage fails entirely.

Have you considered setting an environment variable or whatever the gradle equivalent of an ANT property is to something like package.UnixType = (DebToolsInstalled) ? 'deb' : 'APP_IMAGE? Then all we have to do is figure out how to test for our tools...

Environment property, at worst.

Done, although note that it'll limit what we bundle / what we can target. It's not super useful for me to build binaries for myself, but then again, I have no idea what fraction of our users are on something like Fedora and can't just install debs.

(Also, I bumped the Java version to 11 in this change. Because it's happening sooner or later...)

This detects the existence of commands in the $PATH using the Bash
`which` command.

This also bumps our required version to Java 11 because I didn't want
to pollute the build output, or roll my own NullOutputStream.
heeheehee-kolmafia and others added 4 commits October 6, 2021 19:22
This might be redundant, but it seemed okay in Linux.

This change also replaces instances of " with ' for strings that
didn't use templating.
…lag and also the Mac switches from pkg to dmg.
@gausie
Copy link
Contributor

gausie commented Oct 7, 2021

I still think we shouldn't use "APP_IMAGE". It's not doing anything, we'd get the same result with "BANANA"

@heeheehee-kolmafia
Copy link
Contributor Author

Ah, except BANANA makes the plugin upset.

Cannot convert string value 'banana' to an enum value of type 'org.panteleyev.jpackage.ImageType' (valid case insensitive values: DEFAULT, APP_IMAGE, DMG, PKG, EXE, MSI, RPM, DEB)

This thing won't run on my end without specifying APP_IMAGE, since Oracle thinks all Linux users are on Debian-based distros, so the automatic default for Linux is deb.

@gausie
Copy link
Contributor

gausie commented Oct 7, 2021

Ah this is weird then - on my end I'm getting a different set of valid values, in particular they are lowercase and - separated.

@heeheehee-kolmafia
Copy link
Contributor Author

I think kebab-case is just as valid as SHOUTING_SNAKE_CASE for this plugin. But, best not to mix and match.

Copy link
Contributor

@BadHorseMonkey BadHorseMonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works on MacOSX

It seems like the the Linux problem is more in build.yml than in build.gradle.

But we could add a -PpackageType=deb/rpm/app-image/banana and you can set your type based on that. we could also use that to get dmg and pkg or exe and msi, if we wanted to.

Or we could decide that this version is done and ship it and then do the next patch.

@heeheehee-kolmafia
Copy link
Contributor Author

heeheehee-kolmafia commented Oct 9, 2021

I'm fine insisting that our Linux users on non-deb-based distros figure out how to get java 11 or higher. This is a new service we're providing anyways, and if the deb doesn't work for them, they can keep using the jars as they have been doing.

@heeheehee-kolmafia heeheehee-kolmafia merged commit b30f723 into kolmafia:main Oct 9, 2021
@heeheehee-kolmafia heeheehee-kolmafia deleted the jpackage branch October 9, 2021 13:07
@heeheehee-kolmafia
Copy link
Contributor Author

"KoLmafia-r25762-21.10.25762.dmg"

I feel like we could drop the r25762 part, but it's also okay as it is...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants