Fix build on current GNUstep and CALayer struct-property KVO crash by pkgdemon · Pull Request #8 · gnustep/libs-quartzcore

pkgdemon · 2026-04-23T01:43:45Z

Summary

Two related changes so libs-quartzcore builds and the bundled demo
actually runs on current GNUstep/Linux:

Build fixes so the headers and sources compile against the
current generation of GNUstep core (Foundation/CoreFoundation) and
against a non-Apple OpenGL.
CALayer KVO fix: struct-typed animatable properties
(bounds, position, anchorPoint, transform, sublayerTransform,
contentsRect, shadowOffset) can no longer be registered for
automatic KVO observation on GNUstep — the autogenerated setter
overrides corrupt struct arguments passed by value through
objc_msgSend. The demo app crashed on startup before this.

Build changes (`da7275a`)

Headers/QuartzCore/CABase.h — import Foundation/NSObject.h before
CoreFoundation.h so CF inline functions can resolve -hash /
-isEqual: signatures. (Same pattern libs-opal needs on current
GNUstep.)
Headers/QuartzCore/CATransaction.h — switch from
#import "CoreFoundation/CFDate.h" to
#import <CoreFoundation/CoreFoundation.h>. The quoted per-header
path doesn't resolve reliably against libs-corebase install layout.
Source/CABackingStore.h — move #define GL_GLEXT_PROTOTYPES 1
outside the Apple-only block so non-Apple builds also get it, and
add #import <GL/glext.h> for the non-Apple path.
Source/GNUmakefile / config.make — minor flag adjustments for
the current toolchain.
.gitignore — ignore obj/, derived_src/, *.framework/,
*.app/.

CALayer fix (`2ae02bb`)

CALayer -init used to register every property in its defaults list
for KVO observation via CAImplicitAnimationObserver. On GNUstep this
works for object-typed properties but breaks for struct-typed ones:
the KVO machinery generates a setter override that receives the struct
by value through objc_msgSend, and on GNUstep the argument marshalling
corrupts the struct contents. Setting bounds/position/etc. on a
fresh layer would then either crash or write garbage.

Changes in Source/CALayer.m:

Maintain a separate structKeys list: anchorPoint, transform,
sublayerTransform, contentsRect, shadowOffset, bounds,
position. These are excluded from addObserver:forKeyPath:.
Their setters already emit willChange/didChange via
GSCA_OBSERVABLE_SETTER / the manual -setBounds: / -setPosition:,
so implicit animations still fire.
Add default-value handling for transform and sublayerTransform
analogous to the existing shadowOffset special-case: try KVC first,
fall back to the typed setter inside NS_DURING / NS_HANDLER.
Wrap the remaining (non-struct) addObserver: in NS_DURING / NS_HANDLER
so property types that don't support automatic KVO (e.g. CGColorRef)
don't abort -init.
Drop a dead #if 0 debug block.

Demo changes (`2ae02bb`)

Demo/AppController.m — remove the hand-rolled NSMenu creation
under #if GNUSTEP. The main menu is already provided through the
standard GNUstep app loading path; the duplicate menu setup was a
leftover.
Rename GSQuartzCoreDemo → QuartzCoreDemo in the Info.plist
(ApplicationName, NSExecutable) to match the current bundle name.
Trailing whitespace fix in DemoOpenGLView.m.

Compatibility

macOS/Apple build path: unchanged; all CALayer.m changes are
additive to the existing Apple code paths.
GNUstep with libs-corebase installed: header includes resolve
through <CoreFoundation/CoreFoundation.h> as expected.

Testing

Built cleanly against current GNUstep core (libs-base, libs-gui,
libs-corebase, libs-opal) on Devuan/Linux x86_64.
The bundled Demo/ QuartzCore demo launches, the CALayer tree is
created without crashing, and the root layer renders through the
GL view.

pkgdemon · 2026-04-23T01:45:29Z

Note this depends on gnustep/libs-opal#16

pkgdemon · 2026-04-23T01:58:52Z

@gcasa Here are fixes discussed in last GNUstep meeting.

pkgdemon · 2026-04-25T16:08:19Z

@ivucica Let me know if this is something you might want, or if you foresee the need for a lot of changes. I did get libs-opal fixes merged so this will build now.

pkgdemon · 2026-04-25T16:11:33Z

@ivucica Also something else from the readme I am not sure this is the case anymore:

Patched Opal. Opal currently conflicts with AppKit. More specifically, it also implements an incompatible NSFont. An experimental patch is provided against r35173 of Opal in opal-nsfonthacks.patch.
Opal requires Cairo and may require corebase.

At least with my PR the demo app works fine, and no other patches required.

ivucica

Generally good, except for some confusing codeblock removals and use of forloop where an NSSet would make more sense. Also: fixing GNUstep-specific codepaths should also be marked as GNUstep-specific; there's a decent reason why this should run under macOS (or at least the 13yo versions of it that I was developing this with); checking if a demo behaves the same way natively vs with alt implementation is easier if I don't need a shared directory mounted on two machines (one macOS, one Linux) and can simply use two different build targets on macOS to compare behavior¹.
In future, I'd mainly like seeing fixes split up in multiple PRs independently reviewable. Similarly: bugfix != cleanup != reformatting, and of course != feature work, so (where reasonably possible) it's good to split this up into multiple commits and PRs for reviewability etc.

Comparing behavior strictly for compatibility work, of course. ↩

ivucica · 2026-06-29T11:08:15Z

 {
    ApplicationDescription = "Demonstration of GNUstep QuartzCore";
-    ApplicationName = GSQuartzCoreDemo;
+    ApplicationName = QuartzCoreDemo;


I have a slight preference for not mixing this change in :)

ivucica · 2026-06-29T11:08:42Z

    [_renderer setLayer: layer];
    [layer setBounds: NSRectToCGRect([self bounds])];
-    [layer setBackgroundColor: whiteColor];  
+    [layer setBackgroundColor: whiteColor];


I will accept this, but for the future, I prefer not mixing style changes or cleanup in the same commit or PR as improvements (and same for mixing bugfixes in the same PR as improvements or as cleanups).

ivucica · 2026-06-29T11:11:19Z

 @implementation AppController
 -(void)applicationDidFinishLaunching: (NSNotification*)aNote
 {
-#if GNUSTEP


Why remove this? It's a replacement for a missing .nib / .xib / gorm file. Unless it breaks the build, let's remove this in a separate PR.

This was a few months ago, and no longer fresh in my mind. I will revert back some of these changes locally to see if it did break the build or not, and report back findings.

ivucica · 2026-06-29T11:18:41Z

+         already emit willChange/didChange notifications directly. */
+      static NSString * structKeys[] = {
+        @"anchorPoint", @"transform", @"sublayerTransform",
+        @"contentsRect", @"shadowOffset", @"bounds", @"position" };


We should keep the two lists in sync. Please move this static ... structKeys out to live just next to GSCA_OBSERVABLE_SETTER definition for clarity (and rename it).

Next, this should be an NSSet or similar, for faster lookup. I'll leave a similar comment below.

Next, GSCA_OBSERVABLE_SETTER and that entire part is #if GNUSTEP just like this should be; when built on macOS, this part should not apply.

ivucica · 2026-06-29T11:24:16Z

-          [_observedKeyPaths addObject: keys[i]];
+          /* Set up KVO observation for non-struct properties only */
+          BOOL isStructKey = NO;
+          for (int j = 0; j < sizeof(structKeys)/sizeof(structKeys[0]); j++)


Needlessly introducing O(i*j) ¹; this should be a map/dict or set lookup instead of a for loop (even if the loop is really, really short and probably unrollable by the compiler).

This should also be a GNUstep only bit (#if GNUSTEP). With macOS, all keys should be KVObserved.

Perhaps this does not require a static list of struct keys (and, in fact, might break for user-provided animatable properties, which this implementation can support via @dynamic) -- you could query the runtime.

Footnotes

O notation being 'worst-case', not 'average case' ↩

ivucica · 2026-06-29T11:27:04Z

+                        context: nil];
+              [_observedKeyPaths addObject: keys[i]];
+              NS_HANDLER
+              /* Some property types (e.g. CGColorRef) may not support


I don't like exception handling here. Especially with current implementation that lists supported properties explicitly in the keys variable. If addObserver doesn't like it, we should not blindly ignore it; at the VERY LEAST we should log a warning that something went wrong. Please add at least an NSLog() or equivalent statement. Otherwise, remove the exception handling until we have a need to observe more than keys[i].

ivucica · 2026-06-29T11:27:58Z

-# This file should be autogenerated from config.make.in.
-# However, we have no need for "configure" script yet.


Cleanup unrelated to actual fixes, so this should be a separate commit and even a PR. But I will accept it here.

ivucica · 2026-06-29T11:28:08Z

@@ -0,0 +1,9 @@
+# Build artifacts


Cleanup unrelated to actual fixes, so this should be a separate commit and even a PR. But I will accept it here.

ivucica · 2026-06-29T11:31:12Z


 QuartzCore_OBJCFLAGS += $(WARN_FLAGS)
+# Newer clang treats -Wint-conversion as a hard error; suppress for
+# CoreFoundation.h compatibility shim which has type mismatches in


I have no problem merging this, but should we fix this instead? Is the CoreFoundation.h you speak of in libs-corebase and should that be addressed there? A reference to an issue filed via either Savannah or Github would be great, if fixing there would be appropriate.

(Optional, I don't really want to add too much unnecessary work.)

ivucica · 2026-06-29T11:37:23Z

Sorry for the delay. I'm very glad about the issues identified. I wish I wrote cleaner unit and integration tests that can run headless, but it is what it is.

pkgdemon · 2026-06-29T12:22:12Z

Sorry for the delay. I'm very glad about the issues identified. I wish I wrote cleaner unit and integration tests that can run headless, but it is what it is.

Thanks @ivucica it will take me some time to refresh this with the requested changes. But I can get it done and try to back out some of the changes we may not have needed to get the minimum viable fixes to fix the building, runtime issues. I will try to have something ready ASAP by the end of the week.

ivucica · 2026-06-29T12:29:28Z

 #endif
-#if (__APPLE__)
 #define GL_GLEXT_PROTOTYPES 1
+#if (__APPLE__)


Mostly done as part of merging a variant of PR #7 so during rebase this can be removed. (Import of GL/glext.h is only needed if you also #define GL_GLEXT_LEGACY, see /usr/include/GL/gl.h - in my case, I inspected the one from Debian bookworm from libgl-dev 1.6.0-1).

ivucica · 2026-06-29T12:30:43Z

@pkgdemon No need to hurry, and feel free to push back if something is too large of a request. You can also stack new commits with fixes and just tell me to squash it all at the end.

pkgdemon · 2026-06-30T15:33:05Z

@pkgdemon No need to hurry, and feel free to push back if something is too large of a request. You can also stack new commits with fixes and just tell me to squash it all at the end.

Yes I think part of the reason this was so large is that I combined both build and runtime fixes to make all of the tests work on GNUstep. I did get some other fixes into libs-opal, libs-corebase merged. I'll start fresh and see what was absolutely necessary a little later this week.

pkgdemon added 2 commits April 5, 2026 13:17

Fix build and add gitignore

da7275a

Fixes to CALayer to make demo app work

2ae02bb

pkgdemon requested a review from ivucica as a code owner April 23, 2026 01:43

ivucica requested changes Jun 29, 2026

View reviewed changes

ivucica reviewed Jun 29, 2026

View reviewed changes

		# This file should be autogenerated from config.make.in.
		# However, we have no need for "configure" script yet.

Uh oh!

Conversation

pkgdemon commented Apr 23, 2026

Summary

Build changes (da7275a)

CALayer fix (2ae02bb)

Demo changes (2ae02bb)

Compatibility

Testing

Uh oh!

pkgdemon commented Apr 23, 2026

Uh oh!

pkgdemon commented Apr 23, 2026

Uh oh!

pkgdemon commented Apr 25, 2026

Uh oh!

pkgdemon commented Apr 25, 2026

Uh oh!

ivucica left a comment

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivucica commented Jun 29, 2026

Uh oh!

pkgdemon commented Jun 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivucica commented Jun 29, 2026

Uh oh!

pkgdemon commented Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Build changes (`da7275a`)

CALayer fix (`2ae02bb`)

Demo changes (`2ae02bb`)