Skip to content

sketchybar: switch to apple-sdk_15#352620

Merged
emilazy merged 3 commits intoNixOS:masterfrom
khaneliman:sketchybar
Nov 3, 2024
Merged

sketchybar: switch to apple-sdk_15#352620
emilazy merged 3 commits intoNixOS:masterfrom
khaneliman:sketchybar

Conversation

@khaneliman
Copy link
Contributor

@khaneliman khaneliman commented Oct 31, 2024

Things done

Bump SDK to 15, upstream uses available checks to determine functionality based on available SDK.
ie: https://github.com/FelixKratz/SketchyBar/blob/ed1684d348fc4d4319c7f9835141524c524bacb3/src/window.c#L240-L250

if (__builtin_available(macOS 14.0, *)) {

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Oct 31, 2024
@khaneliman
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 352620


x86_64-darwin

✅ 1 package built:
  • sketchybar

aarch64-darwin

✅ 1 package built:
  • sketchybar

@khaneliman khaneliman changed the title sketchybar: switch to apple-sdk_11 sketchybar: switch to apple-sdk_15 Oct 31, 2024
@khaneliman khaneliman requested a review from emilazy October 31, 2024 23:47
Check version on build and skip calling --help since it doesn't return
version.
@khaneliman
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 352620


x86_64-linux


aarch64-linux


x86_64-darwin

✅ 1 package built:
  • sketchybar

aarch64-darwin

✅ 1 package built:
  • sketchybar

@khaneliman
Copy link
Contributor Author

Updating per new understanding of the codebase and the darwinMinVersionHook. The highest minimum available feature check is SDK 14. Setting that as the minSDK version.

@emilazy
Copy link
Member

emilazy commented Nov 1, 2024

If upstream use availability checks why do we need to set a minimum version?

@khaneliman
Copy link
Contributor Author

khaneliman commented Nov 1, 2024

If upstream use availability checks why do we need to set a minimum version?

Based on our other conversation, I thought the logic was basically "this should be built with the latest SDK available to get all features, but because they have code that relies on SDK 14 it can't be built by anything lower so that's the minimum we set" ?

Does look like Felix sets a bunch of checks though https://github.com/search?q=repo%3AFelixKratz%2FSketchyBar%20__MAC_OS_X_VERSION_MAX_ALLOWED&type=code but I saw these https://github.com/search?q=repo%3AFelixKratz%2FSketchyBar%20__builtin_available(macOS%2014.0%2C%20*)&type=code that matched our reason for setting it to what was gonna compile but it must be fine if we hadn't been building it with a higher SDK before...

@emilazy
Copy link
Member

emilazy commented Nov 1, 2024

Ah, I see the confusion. Okay, let’s have a crack at explaining this again – and I do appreciate getting the opportunity to do so, because we need to figure out how to explain this to packagers!:

  • The SDK version determines what APIs are available. apple-sdk_n requests the SDK for macOS n at minimum, which contains definitions for APIs supported by versions up to and including macOS n. Thus, with apple-sdk_n you cannot target APIs introduced in macOS n + 1, even based on runtime checks.

  • The deployment target or minimum version determines what macOS version is assumed. It gets noted in executable files, and – critically – disables availability checks. So, for instance, if you set the minimum version to 14.0, then you are telling the compiler that you have no interest in running the executable on macOS 13; if (@available(macOS 14, *)) will be a no‐op, and rather than the APIs you use inside such a block being weakly linked and conditioned at runtime based on the running OS version, the executable will simply fail to load on macOS 13 due to missing symbols. If you unconditionally use an API that requires a macOS version newer than your deployment target, you get a compiler warning, but it goes through (and the executable will fail to load on those old versions, unless you do your own weak linking tricks and runtime checks – MoltenVK does this).

  • There is also some kind of notion of a maximum version, but I don’t really know why. It means that using APIs newer than that one will error out, I believe. In practice this is always set to the SDK version for us.

If you want to support building with the macOS 11 SDK, deploying down to 10.15, but optionally using APIs up to macOS 13, then you need to guard your if (@available(macOS 13, *)) blocks with something like #ifdef MAC_OS_VERSION_13_0, because otherwise building with the macOS 12 SDK will fail due to the lack of definitions for those macOS 13 APIs. Sometimes this will look like #if defined(MAC_OS_VERSION_13_0) && MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_VERSION_13_0 when people want to respect the maximum version thing.

tl;dr: Set the SDK so that all used APIs are available, even if they’re behind #ifdef/@available/__builtin_available checks; set the minimum version as low as it can go (whatever upstream says, or failing that the default if it will work, but if there are warnings about using newer APIs without an availability check then you should bump it unless you know they do their own separate runtime checks). Generally you will need to specify the SDK version more often than you need to specify the minimum runtime version.

Please do let me know if anything is still confusing, because honestly it’s fairly confusing and we need to figure out how to explain it to people.

@khaneliman khaneliman force-pushed the sketchybar branch 2 times, most recently from c5d7cb7 to 8de7d45 Compare November 2, 2024 00:29
Upstream uses availability checks for different features. Lets not hold
back functionality by building with latest SDK available.
@khaneliman
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 352620


x86_64-darwin

✅ 1 package built:
  • sketchybar

aarch64-darwin

✅ 1 package built:
  • sketchybar

@khaneliman
Copy link
Contributor Author

@emilazy this one should be good now.

@emilazy emilazy merged commit c5c0a36 into NixOS:master Nov 3, 2024
@khaneliman khaneliman deleted the sketchybar branch November 3, 2024 21:27
@emilazy emilazy mentioned this pull request Nov 26, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants