piegamesde / BlockMap

An out-of-game map renderer and viewer for Minecraft 1.13–1.18 worlds [unmaintained]
MIT License
92 stars 22 forks source link

Support building GUI on macOS #71

Open magicus opened 2 years ago

magicus commented 2 years ago

A small patch for building on macOS. The build basically works, but the jar is named BlockMap-gui-mac os x.jar (with spaces) which is ... well, less than ideal.

I also added automatic building on GitHub Actions. Hopefully you can publish pre-built binaries for macOS as well on future releases on GitHub. :-)

piegamesde commented 2 years ago

Thank you for looking into this. I already noticed this with the "Windows 10" builds, but honestly I didn't care because I'm not scared of spaces in file names. But if you want to fix this, please make it something general that works for all platforms.

magicus commented 2 years ago

@piegamesde I was not aware of any other problematic platforms than macos. Do you have a list of the platforms OS_NAME can take on? (I'm not even sure where it originates from; I knew about os.name but not OS_NAME)

Or do you mean you want me to just replace all spaces with - in the OS_NAME variable? If so, is it still okay to keep the name "macOS" as a special case, since "mac-os-x" would sound weird.

magicus commented 2 years ago

Also, the failing build is not due to this PR. The GHA windows machine got out of memory during regenerate.

piegamesde commented 2 years ago

Or do you mean you want me to just replace all spaces with - in the OS_NAME variable?

I think that would be good, yes. I don't know of any other values other than "Linux" and "Windows 10". But the thing is, this is an environment variable and could be arbitrary.

If so, is it still okay to keep the name "macOS" as a special case

Personally I don't think that the name of the binary is that important, so feel free.

Also, the failing build is not due to this PR. The GHA windows machine got out of memory during regenerate.

Yep. And as of today, Linux fails as well with apparently some certificate failure :roll_eyes:

magicus commented 2 years ago

I finally figured out where OS_NAME is coming from. JavaFX sets it:

// These variables indicate what platform is running the build. Is
// this build running on a Mac, Windows, or Linux machine? 32 or 64 bit?
ext.OS_NAME = System.getProperty("os.name").toLowerCase()
ext.OS_ARCH = System.getProperty("os.arch")
ext.IS_64 = OS_ARCH.toLowerCase().contains("64")
ext.IS_MAC = OS_NAME.contains("mac") || OS_NAME.contains("darwin")
ext.IS_WINDOWS = OS_NAME.contains("windows")
ext.IS_LINUX = OS_NAME.contains("linux")

Maybe we should look for IS_MAC, IS_WINDOWS and IS_LINUX instead? Or use some other method of determining OS?

Or, if you want to, I could keep with the original plan of substituting space with dash, but it seems somewhat fragile to me.

piegamesde commented 2 years ago

I think getting the property directly would be already an improvement, since we would not lose the casing anymore. Matching for IS_XXX is a good idea, because it abstracts the specific OS version away. If it's none of the above, use the property as fallback (or maybe "other". That case probably won't ever happen, because I don't think BlockMap would even build on anything else).