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

Update: generate Mac bundle on install rather than packaging #8189

Closed
wants to merge 2 commits into from

Conversation

@danchr
Copy link
Contributor

@danchr danchr commented Jun 5, 2020

This is a major overhaul of how OpenTTD is packaged for macOS. Rather than using a disk image, it's now a Zip archive. The primary motiviation for this was:

  1. Disk images are annoying.
  2. Technically, creating the bundle is part of the build.

I also added an option for disabling the app bundle “fixup”. This should be useful for developers or packagers such as MacPorts — which was how I started looking at this.

This is a major overhaul of how OpenTTD is packaged for macOS. Rather
than using a disk image, it's now a Zip archive. The primary
motiviation for this was:

1) Disk images are annoying.
2) Technically, creating the bundle _is_ part of the build.

I also added an option for disabling the app bundle "fixup". This
should be useful for developers or packagers such as MacPorts -- which
was how I started looking at this.
@danchr danchr force-pushed the cmake-mac-bundle-on-build branch from 03bbf83 to 8e9afdf Jun 5, 2020
@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jun 5, 2020

Is .dmg not the macOS convention any more?

@danchr
Copy link
Contributor Author

@danchr danchr commented Jun 5, 2020

Is .dmg not the macOS convention any more?

I can't speak to this authoritatively, but I always found them a bit annoying. Since this is actually getting into UX, I tried to find a some references for my hunch:

Apple's guide only mentions archives and installer packages. Safari transparently extracts archives, and the Installer offers to move packages to the trash after processing, so either leads to a more pleasant experience in my opinion.

Unfortunately, all that won't matter much unless the product is signed somehow, as that triggers a dialog that prevents launching the application unless you right-click it.

One other advantage is that creating an archive is faster than creating the disk image.

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jun 5, 2020

I'm not aware of any advantage of .dmg other than convention. Historically they were used to preserve the resource forks, folder layout etc. I don't know if that still holds.

I found the same links you did, and nothing current from Apple recommending .dmg.

src/os/unix/unix.cpp Outdated Show resolved Hide resolved
@glx22
Copy link
Contributor

@glx22 glx22 commented Jun 5, 2020

I see you mentioned "fixup", it looks like nightly build fails at this step.
Maybe you can help me to understand what's wrong.

@danchr danchr force-pushed the cmake-mac-bundle-on-build branch 2 times, most recently from 4eab426 to d2312c8 Jun 18, 2020
@danchr
Copy link
Contributor Author

@danchr danchr commented Jun 18, 2020

I see you mentioned "fixup", it looks like nightly build fails at this step.
Maybe you can help me to understand what's wrong.

Well, it does appear to run into some permissions issues after copying dependencies — which seems a bit odd. What are the user accounts used during the build? Does the install happen with the same user as the actual build?

@glx22
Copy link
Contributor

@glx22 glx22 commented Jun 18, 2020

I fixed the issue with b145ee3 but make install is still broken in master branch, and I think this PR will solve that.

@danchr
Copy link
Contributor Author

@danchr danchr commented Jun 18, 2020

I fixed the issue with b145ee3 but make install is still broken in master branch, and I think this PR will solve that.

I pushed a change so that it should try this out on my branch. We'll see if it addresses this. One of the motivations for this change was that CPack seemed a bit crufty and weird compared to generating the bundle during build.

@danchr danchr force-pushed the cmake-mac-bundle-on-build branch from 552858f to 5660101 Jun 18, 2020
@danchr
Copy link
Contributor Author

@danchr danchr commented Jun 19, 2020

I fixed the issue with b145ee3 but make install is still broken in master branch, and I think this PR will solve that.

Nope; the change I pushed yesterday gets the exact same error, sadly. But I've pushed another applying the same fix you did.

@danchr danchr force-pushed the cmake-mac-bundle-on-build branch from 5660101 to 189d401 Jun 19, 2020
@danchr danchr force-pushed the cmake-mac-bundle-on-build branch 2 times, most recently from aabddde to 9488dfa Jul 1, 2020
@danchr
Copy link
Contributor Author

@danchr danchr commented Jul 1, 2020

I've rebased the change onto master; could you please take another look?

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Dec 13, 2020

Sorry it has been a while; we are currently working on migrating our release workflow from Azure Pipelines to GitHub Actions. During so, we also prepared the Mac builds so they can produce ARM builds and fixed several Mac OS related bundle issues.

During this work, we found out that depending on CPack saves us a lot of time and effort. And although I appreciate your stance on not-liking-dmg-files, I am not convinced that adding the additional code (which we would have to maintain) is worth it. The reason we switched to CMake to start with was to avoid having to maintain all the quirks OSes have ourselves :)

In result, I am leaning towards closing this PR in favour of CPack dmg. This is mainly because the arguments you bring forth dmg is not a nice way to distribute are rather thin, and when we look around (as a non-Mac-user), we still see many projects distribute their applications in dmg format.
This is not a: we-are-never-going-to-do-this, but currently I do not see in the balance of "more custom code" vs "do what-ever CPack does" why the former should win :) I can be convinced otherwise ;)

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

Successfully merging this pull request may close these issues.

None yet

5 participants