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

Provide automatic macOS releases with Apple Silicon support #14839

Merged
merged 9 commits into from
Sep 17, 2021
Merged

Provide automatic macOS releases with Apple Silicon support #14839

merged 9 commits into from
Sep 17, 2021

Conversation

vit9696
Copy link
Contributor

@vit9696 vit9696 commented Sep 12, 2021

This change implements FAT (arm64 + x86_64) binary generation for macOS with custom ./b.sh commands, allowing the resulting PPSSPPSDL.app to run on both Intel and Apple Silicon macOS instances. The resulting binaries:

  • will be uploaded to GitHub artifacts as usual on per-commit basis.
  • will cause a GitHub release creation if a new tag is pushed.

I committed the changeset from #13771 to include an M1-compatible MoltenVK implementation without any changes. The SDL2.framework comes from the official releases unmodified.

A release example will look as follows. I believe one should link to this location on ppsspp.org to make macOS builds clearly accessible.

closes #11799
closes #13708
closes #14815

@hrydgard
Copy link
Owner

Sweet!

But, regarding checking in all the SDL headers... Little reluctant to just have them in the repo like that, rather than a submodule or something. Hm.

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 12, 2021

Well, this is literally just an extraction of SDL2-2.0.16.dmg. I believe I can make the CI download and extract it, but I am not sure whether it is worth the effort. Doing this will:

  • bring manual downloading requirements;
  • add dmg extraction code, which is a bit convoluted as dmg needs mounting;
  • complicate b.sh interface, which would require specifying SDL.framework path;
  • complicate macbundle.sh, which would need to bridge with b.sh.

It would also not help updating SDL as there is no "latest" link from what I can tell. Furthermore, the users get a much easier manual compilation the way I did it as they no longer need brew and have no reason to think about SDL installation separately.

The current solution is not great, but the amount of downsides appearing from doing it the other way around makes me feel we are best to preserve what we have at least for the nearest future.

Maybe we can just put this directory to a hrydgard/ppsspp-sdl submodule? This sounds like a fair solution to me.

Copy link
Collaborator

@unknownbrackets unknownbrackets left a comment

Choose a reason for hiding this comment

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

If there's a way for us to at least use a submodule for the .h files, even if a script has to copy them in the right place, that might make it easier to update. Agreed it's not great to add all the SDL headers. This is just so a user compiling it wouldn't need to download SDL headers on their own?

Can we just use something like https://stackoverflow.com/a/4133639?

I'm not sure if my experience is unique, but I've never really found it a particular challenge to install development libraries on macOS. It's a pain on Windows mainly, because everyone has :opinions: on where they ought to go and no one can manage to have the same opinions or opinions that are generally compatible with where Windows normally puts files.

But on macOS, it's all pretty straight forward - or so I thought? Brew does that weird thing with Cellar or whatever that reminds me of CVS's Attic, but it generally uses /usr/local/ just like packages or anyone else.

-[Unknown]

if: startsWith(github.ref, 'refs/tags/') && runner.os == 'macOS' && matrix.extra == 'test'
with:
files: ppsspp/*.zip
body: >
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not really a fan of creating releases with generic messages for each release. That's exactly the terrible practice Android apps have started using, and I hate it there too.

-[Unknown]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it feels natural to me to edit the release message afterwards with e.g. a changelog manually. This code is just a convenience message to have something before this happens. There is no restriction to the amount of edits, so I feel there is no big issue in leaving it as is, especially since there is no good place to source the release message automatically anyway. Correct me if you think I am wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I don't think anyone will edit them, but okay. Honestly, I'd prefer the release notes were just the version again like now:
https://github.com/hrydgard/ppsspp/releases

Rather than a message that repeats for every single release. Those are not "release notes", those are a description (this repo already has one of those.)

-[Unknown]

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 13, 2021

Hmm. Just to make it clear, on macOS libraries are often distributed as frameworks. A framework is essentially a directory with a shared library, however, it may not be just the shared library. It is common sense to additionally have resource files and interface headers right inside the framework, and this is exactly how SDL is distributed.

Following the suggestion I will just replace SDL/macOS with e.g. ext/SDL submodule, that would contain the macOS directory in its root and all the contents currently pushed here relative to it. Sounds good?


GIT_VERSION_LINE=$(grep "PPSSPP_GIT_VERSION = " "$(dirname "${0}")/../git-version.cpp")
echo "Setting version to ${GIT_VERSION_LINE}..."
Copy link
Contributor Author

@vit9696 vit9696 Sep 13, 2021

Choose a reason for hiding this comment

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

I am worried about the version string in the builds. Currently there seem to be 2 bugs:

  1. The generated version string only has the commit hash and not the version number (1.2.3). See this print from the log.
  2. How can I make sure there is no commit suffix in the tagged releases? I.e. I have the 1.2.3 version name alone if it is a tagged build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe fixed (1) in https://github.com/vit9696/ppsspp/commit/ed046aebf5608869fd49fb55204715b01bc5620b. I am not sure what route shall I take to fix (2). Is it alright to make git-version.cpp with PPSSPP_GIT_VERSION_NO_UPDATE set to 1 in one of the CI stages for tagged commits?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess that's fine, as long as it has the right values. I would think you'd need to fetch tags, i.e. git fetch --tags or more ideally git fetch <remote> 'refs/tags/*:refs/tags/* to be able to have git describe fetch the right thing. That would probably fetch less and be faster.

-[Unknown]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that, but it did not work for me. I believe the reason is that we not only need to fetch the tags, but also connect tags to the history, otherwise one would get ge1cb1c9a6 instead of v1.11.3-1350-ge1cb1c9a6 for intermediate releases. I changed the code to only fetch history on macOS, however, which should improve the situation quite a bit.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@vit9696
Copy link
Contributor Author

vit9696 commented Sep 13, 2021

@hrydgard @unknownbrackets do you think we can schedule that for 1.12?

@hrydgard
Copy link
Owner

Since we're currently not making official releases for Mac, the timing is not that important.

However I do want to get some form of this in of course, this is a good start.

But can't we rely on like brew-installed SDL, or something? I guess maybe not working when we want to package for distribution?

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 13, 2021

brew only builds for the current architecture, and we need to support both ARM64 and x86_64. For this reason brew is not an option.

We either need to build SDL2 ourselves or use a prebuilt one. The latter is much easier as SDL project offers prebuilt binaries with optimal configuration: they are neatly crafted to target very low macOS versions.

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 15, 2021

@hrydgard are we decided on the SDL inclusion nuance? I personally believe the way it is now is ok at least personally, but that needs a project decision really.

@hrydgard
Copy link
Owner

Yeah, I understand now that we might have little choice, in order to keep the build convenient.

There's still the option of keeping it in a submodule, but on the other hand, we already have some fairly big dependencies in the repo, and even if it's in a submodule, everybody will end up syncing it down anyway.

If @unknownbrackets feels that submodule is better, we can do that, but to me, like I said, I don't see any huge pros.

Regarding the changelog thing, not related to this PR but I had to resort to a generic message on Android after a release got blocked when I mentioned the names of some fixed games in the log (plus it's limited to 500 chars which isn't enough).

Copy link
Collaborator

@unknownbrackets unknownbrackets left a comment

Choose a reason for hiding this comment

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

Okay, if SDL needs to be updated is it just a matter of copying something from an SDL dmg? Just want to make sure if SDL requires an update it won't block a release now or something. A submodule is at least relatively straight-obvious to upgrade.

I personally don't have a macOS device I can use for PPSSPP development anymore, so a submodule also seemed a bit more accessible to me.

-[Unknown]

@@ -959,7 +973,9 @@ else()
set(PNG_REQUIRED_VERSION 1.6)
endif()

if(USE_SYSTEM_LIBPNG)
find_package(PNG ${PNG_REQUIRED_VERSION})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be indented?

-[Unknown]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this one is finding "system" libpng, which on macOS can come from a package manager, such as brew or MacPorts. Doing this will result in linking to a library that does not come with macOS and is thus broken.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just mean mechanically, the find_package(PNG ${PNG_REQUIRED_VERSION}) should have a tab character before it.

-[Unknown]

@unknownbrackets
Copy link
Collaborator

Regarding the changelog thing, not related to this PR but I had to resort to a generic message on Android after a release got blocked when I mentioned the names of some fixed games in the log (plus it's limited to 500 chars which isn't enough).

A bad policy results in bad results... sigh. But this is most apps now, and although there's a good reason in this case I suppose... I consider the general state of release notes a real regression from the past.

I guess everyone's just hoping for the day where it's someone else's device you're renting, that you have no rights for whatsoever, and is entirely in the cloud. That way no one has to bother themselves with owning anything or having any distracting ability to fiddle with things or become a programmer as a child like I did.

Oh well. I shouldn't complain about people getting the future they want if it's the future they want...

-[Unknown]

hrydgard added a commit to hrydgard/ppsspp-mac-sdl that referenced this pull request Sep 16, 2021
@hrydgard
Copy link
Owner

At least we do provide proper release notes on the website, and I refer to it from the Play release notes as a workaround, but it's indeed a shame.

I changed my mind again, let's do the submodule. I created this repository with the contents: https://github.com/hrydgard/ppsspp-mac-sdl

Please just replace the macOS directory under SDL with a submodule pointer to this repository.

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 16, 2021

Done. A bit confused why does the CI fail at the moment though. Is not checkout recursive for test jobs (https://github.com/hrydgard/ppsspp/pull/14839/checks?check_run_id=3618812569)?

@hrydgard
Copy link
Owner

Huh, I would have thought that should have worked. We already rely on multiple submodules...

@unknownbrackets
Copy link
Collaborator

So, doing a shallow (--depth=1) clone into a new directory, and then git describe --always gives the sha (as expected.)

But then doing:

git fetch --deepen=15000 --tags

Does fetch a good chunk of data, but won't mess with submodules. That's enough to get a good git describe --always result.

-[Unknown]

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 16, 2021

@hrydgard are you sure you committed all the symlinks correctly?

@hrydgard
Copy link
Owner

hrydgard commented Sep 16, 2021

@vit9696 I guess maybe I did not, since I did it on Windows! Wasn't aware there were symlinks.

I'll re-do it from Linux or Mac later. Maybe I should just extract SDL instead of copying from this branch.

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 16, 2021

Yes, frameworks heavily rely on symlinks.

@hrydgard
Copy link
Owner

Done, you're gonna have to update the submodule pointer to f19a1d54b8a5af6cc378ea307e0ec676922eb4cc .

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 16, 2021

So, doing a shallow (--depth=1) clone into a new directory, and then git describe --always gives the sha (as expected.)

But then doing:

git fetch --deepen=15000 --tags

Does fetch a good chunk of data, but won't mess with submodules. That's enough to get a good git describe --always result.

-[Unknown]

The CI run seems to disagree: https://github.com/vit9696/ppsspp/runs/3626248500

@hrydgard hrydgard merged commit cd05f3a into hrydgard:master Sep 17, 2021
@unknownbrackets unknownbrackets added this to the v1.12.0 milestone Sep 17, 2021
@EricFromCanada
Copy link

See Endless Sky for another implementation of automatic download & install of SDL with GitHub Actions.

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

Successfully merging this pull request may close these issues.

Binaries for macOS Universal Binary for Apple Silicon Empty macOS Dev-Latest build
5 participants