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

Improved/fixed macOS support #4153

Merged
merged 12 commits into from
Nov 18, 2023

Conversation

DingDongSoLong4
Copy link
Collaborator

@DingDongSoLong4 DingDongSoLong4 commented Sep 25, 2023

This improves macOS support:

  • Desktop notifications appear to be broken, and fixing them required a patch to gosx-notifier (Replace Link with Open and Activate kermieisinthehouse/gosx-notifier#1). This was because the -sender flag was not being used.
  • Distribute a unified (both Intel + ARM) stash-macos binary, rather than separate binaries.
  • Distribute a packaged Stash.app, allowing macOS users to completely avoid the Terminal.
  • Also changed the Bundle ID to match stashapp.cc.

@DingDongSoLong4
Copy link
Collaborator Author

DingDongSoLong4 commented Sep 25, 2023

I've just come across #2284, but for me on macOS Ventura 13.5.2 Stash is able to create and find the config file in $HOME/.stash perfectly fine - not sure if something changed on the macOS side or the Stash side.

Edit: I misread that issue, and I can reproduce it now. The problem is caused by App Translocation, which runs the app from a random temporary directory, making it basically impossible to figure out where the binary is located. The only solution will have to be disabling using current working directory for setup when running the .app.

@kermieisinthehouse
Copy link
Collaborator

Fantastic that someone who can figure out MacOS is looking at this.

Did you figure out how to reduce the executable size with the unified app? When I last tried this, the frontend assets were included twice, so the Mac binary was nearly twice the size of the other platforms.

@DingDongSoLong4
Copy link
Collaborator Author

Did you figure out how to reduce the executable size with the unified app? When I last tried this, the frontend assets were included twice, so the Mac binary was nearly twice the size of the other platforms.

Ah I hadn't thought of size, but I have no idea how, or if it is even possible, to deduplicate data in a unified binary. It makes sense that it would be twice the size (and it indeed is), since it basically contains two separate binaries.

The only real thing that could be deduplicated is the UI assets, as you mention, which are currently 7.8MB. The entire macOS unified binary is 85MB, so that's a potential ~10% decrease, which isn't that much imo.

The alternative is to do what we're doing now and keep the two macOS binaries separate, but I think the convenience of the unified binary is worth the size tradeoff to most users.

And PS I don't actually know that much about macOS apps/development, I just have a MacBook so I'm able to test things - all of the fixes here came from trial, error and Google :)

@kermieisinthehouse
Copy link
Collaborator

85 MB is definitely not catastrophic and I wouldn't let that hinder this.

@WithoutPants WithoutPants added this to the Version 0.24.0 milestone Sep 26, 2023
@WithoutPants WithoutPants added bug Something isn't working improvement Something needed tweaking. labels Oct 26, 2023
@DingDongSoLong4
Copy link
Collaborator Author

Alright, I believe this is now ready.

When running the .app, the working dir seems to always be set to /. If this is detected during setup, choosing a working directory is now disabled:
image
I'm not 100% happy with the message, but it's the best I've been able to come up with while keeping it as short as possible to avoid a truly enormous button.

I've also made a few other improvements to the Setup process, now that the OS and working/home directories are exposed to the UI. The most useful of these is probably that the two initial buttons now show the actual paths they refer to, rather than "In the current working directory" for example. This should hopefully make it more clear where Stash is actually being set up, and should also help if the current directory is set to something unexpected.

@DingDongSoLong4
Copy link
Collaborator Author

DingDongSoLong4 commented Nov 15, 2023

I've added some tweaks/improvements to the Makefile:

  • The use of the flags-* targets is now more explicitly "advertised", which means that the separate stash-release, stash-release-static, etc. targets are no longer necessary. Only the build-release target is kept for convenience. This means we won't have to add a bunch of extra targets when e.g. a stashpkg binary is added.
  • The cross-compile-* targets have been renamed to build-cc-*. This is in an attempt to emphasize that they are cross-compile versions of the build target only, and make ui, make generate, etc. are still required when using them.
  • -applesilicon is now -arm.
  • The ui-nolegacy and ui-sourcemaps have been removed - they never actually worked, since ifdefs are evaluated at the initial parsing stage, not when actually executing a recipe.

There's also now a new stash-macapp target which lets you build Stash.app when running on a Mac, without having to cross-compile.

I've also removed cross-compile.sh and upload-pull-request.sh. upload-pull-request.sh hasn't been used in a long time, and cross-compile.sh is very outdated and not really necessary.

And then I've updated various documentation files, to reflect these changes and remove outdated information.

@WithoutPants WithoutPants merged commit 4dd4c3c into stashapp:develop Nov 18, 2023
2 checks passed
@DingDongSoLong4 DingDongSoLong4 deleted the macos-desktop-fixes branch November 19, 2023 00:42
DingDongSoLong4 added a commit that referenced this pull request Nov 21, 2023
@DingDongSoLong4 DingDongSoLong4 mentioned this pull request Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working improvement Something needed tweaking.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants