bug#27810: NS runtime feature detection

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Alan Third
I wrote a big reply to this but seem to have lost it.

On Mon, Jul 24, 2017 at 09:02:57PM +0200, Charles A. Roelli wrote:
> nsterm.m: In function ‘-[EmacsView windowDidEnterFullScreen]’:
> nsterm.m:7395: error: ‘NSApplicationPresentationFullScreen’ undeclared
> (first use in this function)
> nsterm.m:7395: error: (Each undeclared identifier is reported only once
> nsterm.m:7395: error: for each function it appears in.)
> nsterm.m:7396: error: ‘NSApplicationPresentationAutoHideToolbar’ undeclared
> (first use in this function)

I think perhaps we need to just test ‘< 10.7’. I’ve attached a new
patch that deals with that and some other bits and pieces.

> I'm confused why the macro call you wrote doesn't prevent this. But
> when I change it to #if MAC_OS_X_VERSION_MIN_ALLOWED <=
> MAC_OS_X_VERSION_10_6, then it compiles.  This min/max stuff always
> confuses me...

I’m unclear where we should be using MIN_REQUIRED vs MAX_ALLOWED, but
I think we’re OK with MAX everywhere for what we’re doing. I’ve just
used MAX in both my new macros.

We need a proper runtime OS version check in a few places. I think
there are two ways of doing this depending on which OS version you’re
building on. It’s getting very close to circular. ;)

macfont.m looks like it could be a small nightmare as it has a LOT of
version dependent code.

Thanks for testing this patch.
--
Alan Third

0001-Use-a-run-time-check-for-macOS-Sierra-tabbing-suppor.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Glenn Morris-3
Alan Third wrote:

> I think perhaps we need to just test '< 10.7'. I've attached a new
> patch that deals with that and some other bits and pieces.
[...]

> I'm unclear where we should be using MIN_REQUIRED vs MAX_ALLOWED, but
> I think we're OK with MAX everywhere for what we're doing. I've just
> used MAX in both my new macros.
>
> We need a proper runtime OS version check in a few places. I think
> there are two ways of doing this depending on which OS version you're
> building on. It's getting very close to circular. ;)
>
> macfont.m looks like it could be a small nightmare as it has a LOT of
> version dependent code.


FWIW, a comment from someone who is disinterested in OS X:

You could probably make your life easier by not supporting more
than Apple does. Eg on the net I read:

    Apple is only supporting Mac OS X 10.9 – 10.11. Apple stopped
    providing security updates for Mac OS 10.8 and earlier versions with
    the release of Mac OS X 10.11 (El Capitan) in 2015.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Alan Third
On Mon, Jul 24, 2017 at 04:53:02PM -0400, Glenn Morris wrote:
> FWIW, a comment from someone who is disinterested in OS X:
>
> You could probably make your life easier by not supporting more
> than Apple does. Eg on the net I read:
>
>     Apple is only supporting Mac OS X 10.9 – 10.11. Apple stopped
>     providing security updates for Mac OS 10.8 and earlier versions with
>     the release of Mac OS X 10.11 (El Capitan) in 2015.

It would make life a lot easier, we could get rid of almost all
version dependent code. Just dropping support for 10.6 would remove a
lot too.

I doubt Charles would like that, though, and he’s the only other
person regularly working on the NS port.
--
Alan Third



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Charles A. Roelli
I'm not attached to 10.6, but if we can keep supporting it, I'd like to.


And for what it's worth, there are 11 places in nsterm.m with
version-specific code (unless I've missed some):

12 matches for "MAC_OS_X_VERSION_MAX_ALLOWED" in buffer: nsterm.m
     140:#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7
     157:#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7
    5567:  MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_9
    6329:  MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
    7036:  MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_9
    7098:  MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_9
    7365:#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7
    7516:  MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_9
    7547:  MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_9
    8109:#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_9
    8120:#endif /* MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_9 */
    8327:  MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7

which seems manageable.  But if and when support for earlier macOSen
causes problems, then I'd have no objections removing support.


On 25/07/2017 19:56, Alan Third wrote:

> On Mon, Jul 24, 2017 at 04:53:02PM -0400, Glenn Morris wrote:
>> FWIW, a comment from someone who is disinterested in OS X:
>>
>> You could probably make your life easier by not supporting more
>> than Apple does. Eg on the net I read:
>>
>>      Apple is only supporting Mac OS X 10.9 – 10.11. Apple stopped
>>      providing security updates for Mac OS 10.8 and earlier versions with
>>      the release of Mac OS X 10.11 (El Capitan) in 2015.
> It would make life a lot easier, we could get rid of almost all
> version dependent code. Just dropping support for 10.6 would remove a
> lot too.
>
> I doubt Charles would like that, though, and he’s the only other
> person regularly working on the NS port.




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Anders Lindgren-2
Hi!

There are a lot of Mac:s still used where the last supported OS version is 10.6.8. It would be a nice touch to support them, at least as long as all we need to come up with is a good set of preprocessor tests to exclude code. Of course, should we need to put a lot of effort into maintaining support, I too would vote for dropping them (after all, there are fairly modern versions of Emacs available for them).

    -- Anders

On Tue, Jul 25, 2017 at 8:22 PM, Charles A. Roelli <[hidden email]> wrote:
I'm not attached to 10.6, but if we can keep supporting it, I'd like to.


And for what it's worth, there are 11 places in nsterm.m with
version-specific code (unless I've missed some):

12 matches for "MAC_OS_X_VERSION_MAX_ALLOWED" in buffer: nsterm.m
    140:#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7
    157:#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7
   5567:  MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_9
   6329:  MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
   7036:  MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_9
   7098:  MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_9
   7365:#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7
   7516:  MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_9
   7547:  MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_9
   8109:#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_9
   8120:#endif /* MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_9 */
   8327:  MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7

which seems manageable.  But if and when support for earlier macOSen
causes problems, then I'd have no objections removing support.



On 25/07/2017 19:56, Alan Third wrote:
On Mon, Jul 24, 2017 at 04:53:02PM -0400, Glenn Morris wrote:
FWIW, a comment from someone who is disinterested in OS X:

You could probably make your life easier by not supporting more
than Apple does. Eg on the net I read:

     Apple is only supporting Mac OS X 10.9 – 10.11. Apple stopped
     providing security updates for Mac OS 10.8 and earlier versions with
     the release of Mac OS X 10.11 (El Capitan) in 2015.
It would make life a lot easier, we could get rid of almost all
version dependent code. Just dropping support for 10.6 would remove a
lot too.

I doubt Charles would like that, though, and he’s the only other
person regularly working on the NS port.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Alan Third
In reply to this post by Alan Third
On Mon, Jul 24, 2017 at 09:44:04PM +0100, Alan Third wrote:
> > I'm confused why the macro call you wrote doesn't prevent this. But
> > when I change it to #if MAC_OS_X_VERSION_MIN_ALLOWED <=
> > MAC_OS_X_VERSION_10_6, then it compiles.  This min/max stuff always
> > confuses me...
>
> I’m unclear where we should be using MIN_REQUIRED vs MAX_ALLOWED, but
> I think we’re OK with MAX everywhere for what we’re doing. I’ve just
> used MAX in both my new macros.

I think I finally worked it out by reading macfont.m. I’ve attached
YET ANOTHER version of this which doesn't use any custom macros.

To compile for multiple versions you do something like:

./configure --with-ns CFLAGS="-DMAC_OS_X_VERSION_MIN_REQUIRED=1070 -DMAC_OS_X_VERSION_MAX_ALLOWED=101200 -g -O3"

By default max and min are set to the version you’re running on,
afaict. Please be aware that 10.6 isn’t fully compatible with this at
the moment, as there is at least one place (nsmenu.m:535) where
there’s a bug fix for it that I can’t see an immediate way of making
dynamic.

Additionally the native fullscreen stuff is based on a simple check
against MAX. I expect I’ll be able to fix that with a bit of work,
though.

I’d be interested to see if this builds on systems lower than 10.10
with the above configure command. There are probably bits and pieces
I’ve not quite got right.

--
Alan Third

0001-Allow-use-of-run-time-OS-version-checks-on-macOS-bug.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Charles A. Roelli
Shouldn't MIN_ALLOWED be MIN_REQUIRED?  As in here:

+#if defined (NS_IMPL_COCOA) \
+  && MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7
+#if MAC_OS_X_VERSION_MIN_ALLOWED < MAC_OS_X_VERSION_10_7

There are a few more places that have a MIN_ALLOWED thing.  This
always trips me up, so I'm not sure.

And also, it's apparently feasible to do a runtime check for a
specific macOS, according to this:

https://developer.apple.com/library/content/documentation/DeveloperTools/Conceptual/cross_development/Using/using.html#//apple_ref/doc/uid/20002000-SW7

which mentions APPKIT_EXTERN double NSAppKitVersionNumber.  I'm not
sure if they still define this on Sierra, though.  But if they do,
then we can use this to fix the nsmenu.m problem.


On 26/07/2017 23:57, Alan Third wrote:

> On Mon, Jul 24, 2017 at 09:44:04PM +0100, Alan Third wrote:
>>> I'm confused why the macro call you wrote doesn't prevent this. But
>>> when I change it to #if MAC_OS_X_VERSION_MIN_ALLOWED <=
>>> MAC_OS_X_VERSION_10_6, then it compiles.  This min/max stuff always
>>> confuses me...
>> I’m unclear where we should be using MIN_REQUIRED vs MAX_ALLOWED, but
>> I think we’re OK with MAX everywhere for what we’re doing. I’ve just
>> used MAX in both my new macros.
> I think I finally worked it out by reading macfont.m. I’ve attached
> YET ANOTHER version of this which doesn't use any custom macros.
>
> To compile for multiple versions you do something like:
>
> ./configure --with-ns CFLAGS="-DMAC_OS_X_VERSION_MIN_REQUIRED=1070 -DMAC_OS_X_VERSION_MAX_ALLOWED=101200 -g -O3"
>
> By default max and min are set to the version you’re running on,
> afaict. Please be aware that 10.6 isn’t fully compatible with this at
> the moment, as there is at least one place (nsmenu.m:535) where
> there’s a bug fix for it that I can’t see an immediate way of making
> dynamic.
>
> Additionally the native fullscreen stuff is based on a simple check
> against MAX. I expect I’ll be able to fix that with a bit of work,
> though.
>
> I’d be interested to see if this builds on systems lower than 10.10
> with the above configure command. There are probably bits and pieces
> I’ve not quite got right.
>




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Anders Lindgren-2
It's always a good idea to enable warnings when undefined preprocessor symbols are used. In gcc this is -Wundef and I gess it's the same in clang.

    -- Anders

On Mon, Jul 31, 2017 at 9:05 PM, Charles A. Roelli <[hidden email]> wrote:
Shouldn't MIN_ALLOWED be MIN_REQUIRED?  As in here:

+#if defined (NS_IMPL_COCOA) \
+  && MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7
+#if MAC_OS_X_VERSION_MIN_ALLOWED < MAC_OS_X_VERSION_10_7

There are a few more places that have a MIN_ALLOWED thing.  This
always trips me up, so I'm not sure.

And also, it's apparently feasible to do a runtime check for a
specific macOS, according to this:

https://developer.apple.com/library/content/documentation/DeveloperTools/Conceptual/cross_development/Using/using.html#//apple_ref/doc/uid/20002000-SW7

which mentions APPKIT_EXTERN double NSAppKitVersionNumber.  I'm not
sure if they still define this on Sierra, though.  But if they do,
then we can use this to fix the nsmenu.m problem.



On 26/07/2017 23:57, Alan Third wrote:
On Mon, Jul 24, 2017 at 09:44:04PM +0100, Alan Third wrote:
I'm confused why the macro call you wrote doesn't prevent this. But
when I change it to #if MAC_OS_X_VERSION_MIN_ALLOWED <=
MAC_OS_X_VERSION_10_6, then it compiles.  This min/max stuff always
confuses me...
I’m unclear where we should be using MIN_REQUIRED vs MAX_ALLOWED, but
I think we’re OK with MAX everywhere for what we’re doing. I’ve just
used MAX in both my new macros.
I think I finally worked it out by reading macfont.m. I’ve attached
YET ANOTHER version of this which doesn't use any custom macros.

To compile for multiple versions you do something like:

./configure --with-ns CFLAGS="-DMAC_OS_X_VERSION_MIN_REQUIRED=1070 -DMAC_OS_X_VERSION_MAX_ALLOWED=101200 -g -O3"

By default max and min are set to the version you’re running on,
afaict. Please be aware that 10.6 isn’t fully compatible with this at
the moment, as there is at least one place (nsmenu.m:535) where
there’s a bug fix for it that I can’t see an immediate way of making
dynamic.

Additionally the native fullscreen stuff is based on a simple check
against MAX. I expect I’ll be able to fix that with a bit of work,
though.

I’d be interested to see if this builds on systems lower than 10.10
with the above configure command. There are probably bits and pieces
I’ve not quite got right.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Alan Third
On Tue, Aug 01, 2017 at 05:38:03PM +0200, Anders Lindgren wrote:
> It's always a good idea to enable warnings when undefined preprocessor
> symbols are used. In gcc this is -Wundef and I gess it's the same in clang.

Unfortunately this produces an absolute ton of spurious warnings which
scroll off my terminal buffer. I think I’ve caught all the important
ones now, though.

I’ve attached my latest go with this. I’ve removed the
MAC_OS_X_VERSION_10_XX macros with their numbers, as we can use the
existence of the macros to tell what platform we’re compiling on. Eg.

    #if !defined (MAC_OS_X_VERSION_10_7) \
      && MAC_OS_X_VERSION_MAX_ALLOWED >= 1070

I’ve also used the NSAppKitVersionNumber macros in a few places to do
runtime version detection. I’m not sure this is reliable. There are
two methods of finding the OS version definitively, which we can use
if we need to.

I’ve got rid of the HAS_NATIVE_FS macro and tried to replace it with
other checks where required. It compiles on 10.12, but I’m not at all
convinced it will compile on 10.6.

New 10.7 variables that need to be available on 10.6 when we’re
compiling for 10.7+ have been added near the bottom of nsterm.h.

Overall it looks quite different in places, but the functionality
hasn’t really changed.
--
Alan Third

0001-Allow-use-of-run-time-OS-version-checks-on-macOS-bug.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Charles A. Roelli
I ran a simple configure/compile with your patch installed, which worked
fine.
I then tried:

./configure --with-ns CFLAGS=-DMAC_OS_X_VERSION_MIN_REQUIRED=1060
-DMAC_OS_X_VERSION_MAX_ALLOWED=101200 -g -O3

and ran into a few errors, which should be fixed with the attached
patch applied on top of yours.  I've written notes on some of the
changed parts below.

--- a/src/macfont.h
+++ b/src/macfont.h
@@ -45,12 +45,12 @@ struct mac_glyph_layout
    CGGlyph glyph_id;
  };

-#if MAC_OS_X_VERSION_MAX_ALLOWED < 1080
+#if !defined (MAC_OS_X_VERSION_10_8)

We have to define these constants when compiling on macOS < 10.8,
since they're used by macfont.m and only available on 10.8+.

-#if MAC_OS_X_VERSION_MAX_ALLOWED >= 1070
+#if MAC_OS_X_VERSION_MAX_ALLOWED >= 1070 && defined (MAC_OS_X_VERSION_10_7)
    kCTFontTraitColorGlyphs = kCTFontColorGlyphsTrait
  #else
    kCTFontTraitColorGlyphs = (1 << 13)

kCTFontColorGlyphsTrait is only defined on macOS 10.7+.

--- a/src/macfont.m
+++ b/src/macfont.m
@@ -2875,7 +2875,7 @@ So we use
CTFontDescriptorCreateMatchingFontDescriptor (no
@selector(backingScaleFactor)])
  #endif
              CGContextSetLineWidth (context, synthetic_bold_factor *
font_size
-                                   * [[FRAME_NS_VIEW(f) window]
backingScaleFactor]);
+                                   * [(EmacsWindow *) [FRAME_NS_VIEW(f)
window] backingScaleFactor]);

Compiler needs a cast to EmacsWindow * here.  I add the backing scale
factor to the interface declaration of EmacsWindow here in nsterm.h:

@@ -470,6 +499,10 @@ @interface EmacsWindow : NSWindow
  {
    NSPoint grabOffset;
  }
+#if !defined (MAC_OS_X_VERSION_10_7) && MAC_OS_X_VERSION_MAX_ALLOWED >=
1070
+- (NSRect)convertRectToScreen:(NSRect)rect;
+@property(readonly) CGFloat backingScaleFactor;
+#endif
  @end

Next, in nsfns.m:

--- a/src/nsfns.m
+++ b/src/nsfns.m
@@ -1592,7 +1592,7 @@ Frames are listed from topmost (first) to
bottommost (last).  */)
  }

  #ifdef NS_IMPL_COCOA
-#if MAC_OS_X_VERSION_MAX_ALLOWED > 1090
+#if MAC_OS_X_VERSION_MAX_ALLOWED > 1090 && defined (MAC_OS_X_VERSION_10_9)
  #define MODAL_OK_RESPONSE NSModalResponseOK
  #endif
  #endif

NSModelResponseOK is only defined on macOS 10.9+.  The next ifndef
clause takes care of the right define on macOS below 10.9.

Next, in src/nsterm.h:

+#define NSAppKitVersionNumber10_7 1138

New define, since it's referenced verbatim in nsterm.m:7017:

#if MAC_OS_X_VERSION_MIN_REQUIRED < 1070
   if (NSAppKitVersionNumber >= NSAppKitVersionNumber10_7)
#endif

New scroll styles in 10.7+:

+enum {
+    NSScrollerStyleLegacy = 0,
+    NSScrollerStyleOverlay = 1
+};
+typedef NSInteger NSScrollerStyle;

Add-on to the class declaration of NSScroller (not sure if this is the
right way to do it).  Otherwise the compiler errors out on compiling
the 10.7+ call to scrollerWidthForControlSize:

+@interface NSScroller(NSObject)
++ (CGFloat)scrollerWidthForControlSize:(NSControlSize)controlSize
scrollerStyle:(NSScrollerStyle)scrollerStyle;
+@end

Forward declarations for functions used by macfont.m (declared as weak
imports since they won't all be available unless we're on 10.8+):

+void CTFontDrawGlyphs(CTFontRef font, const CGGlyph *glyphs, const
CGPoint *positions, size_t count, CGContextRef context)
__attribute__((weak_import));
+#endif
+
+#if !defined (MAC_OS_X_VERSION_10_8) && MAC_OS_X_VERSION_MAX_ALLOWED >=
1080
+extern CFArrayRef CTFontCopyDefaultCascadeListForLanguages(CTFontRef
font, CFArrayRef languagePrefList) __attribute__((weak_import));

The compiler issued no complaints here, but the linker would not link
temacs unless the symbols were listed as permitted to be undefined,
using this in src/Makefile:

## System-specific LDFLAGS.
LD_SWITCH_SYSTEM= -Wl,-U,_CTFontCopyDefaultCascadeListForLanguages
-Wl,-U,_CTFontDrawGlyphs

I'm not sure where to integrate this in the source tree (and if we can
conditionalize it based on what version of macOS we're building for).

If they look okay, could you please integrate these changes into your
patch?  Thanks a lot for your help on this.


On 02/08/2017 00:03, Alan Third wrote:

> On Tue, Aug 01, 2017 at 05:38:03PM +0200, Anders Lindgren wrote:
>> It's always a good idea to enable warnings when undefined preprocessor
>> symbols are used. In gcc this is -Wundef and I gess it's the same in clang.
> Unfortunately this produces an absolute ton of spurious warnings which
> scroll off my terminal buffer. I think I’ve caught all the important
> ones now, though.
>
> I’ve attached my latest go with this. I’ve removed the
> MAC_OS_X_VERSION_10_XX macros with their numbers, as we can use the
> existence of the macros to tell what platform we’re compiling on. Eg.
>
>      #if !defined (MAC_OS_X_VERSION_10_7) \
>        && MAC_OS_X_VERSION_MAX_ALLOWED >= 1070
>
> I’ve also used the NSAppKitVersionNumber macros in a few places to do
> runtime version detection. I’m not sure this is reliable. There are
> two methods of finding the OS version definitively, which we can use
> if we need to.
>
> I’ve got rid of the HAS_NATIVE_FS macro and tried to replace it with
> other checks where required. It compiles on 10.12, but I’m not at all
> convinced it will compile on 10.6.
>
> New 10.7 variables that need to be available on 10.6 when we’re
> compiling for 10.7+ have been added near the bottom of nsterm.h.
>
> Overall it looks quite different in places, but the functionality
> hasn’t really changed.


0001-draft-macos-runtime-check.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Alan Third
On Sun, Aug 06, 2017 at 10:29:49PM +0200, Charles A. Roelli wrote:

> I ran a simple configure/compile with your patch installed, which worked
> fine.
> I then tried:
>
> ./configure --with-ns CFLAGS=-DMAC_OS_X_VERSION_MIN_REQUIRED=1060
> -DMAC_OS_X_VERSION_MAX_ALLOWED=101200 -g -O3
>
> and ran into a few errors, which should be fixed with the attached
> patch applied on top of yours.  I've written notes on some of the
> changed parts below.

Thanks! These are exactly the kinds of errors I was expecting to see.

> Next, in src/nsterm.h:
>
> +#define NSAppKitVersionNumber10_7 1138
>
> New define, since it's referenced verbatim in nsterm.m:7017:

Ah, I didn’t even think of that. :)

> New scroll styles in 10.7+:
>
> +enum {
> +    NSScrollerStyleLegacy = 0,
> +    NSScrollerStyleOverlay = 1
> +};
> +typedef NSInteger NSScrollerStyle;

I believe we can make this slightly neater:

    enum NSScrollerStyle {
      NSScrollerStyleLegacy = 0,
      NSScrollerStyleOverlay = 1
    };

> Add-on to the class declaration of NSScroller (not sure if this is the
> right way to do it).  Otherwise the compiler errors out on compiling
> the 10.7+ call to scrollerWidthForControlSize:
>
> +@interface NSScroller(NSObject)
> ++ (CGFloat)scrollerWidthForControlSize:(NSControlSize)controlSize
> scrollerStyle:(NSScrollerStyle)scrollerStyle;
> +@end

Strange... Are you sure this one was an error and not just a warning?
We’re expecting warnings for this type of thing.

> Forward declarations for functions used by macfont.m (declared as weak
> imports since they won't all be available unless we're on 10.8+):
>
> +void CTFontDrawGlyphs(CTFontRef font, const CGGlyph *glyphs, const CGPoint
> *positions, size_t count, CGContextRef context)
> __attribute__((weak_import));
> +#endif
> +
> +#if !defined (MAC_OS_X_VERSION_10_8) && MAC_OS_X_VERSION_MAX_ALLOWED >=
> 1080
> +extern CFArrayRef CTFontCopyDefaultCascadeListForLanguages(CTFontRef font,
> CFArrayRef languagePrefList) __attribute__((weak_import));
>
> The compiler issued no complaints here, but the linker would not link
> temacs unless the symbols were listed as permitted to be undefined,
> using this in src/Makefile:
>
> ## System-specific LDFLAGS.
> LD_SWITCH_SYSTEM= -Wl,-U,_CTFontCopyDefaultCascadeListForLanguages
> -Wl,-U,_CTFontDrawGlyphs

I’ve done a bit more reading up on this and I think I’ve misunderstood
how this works, and probably mislead you.

It seems these functions need to be declared as weak in the definition
of the library they’re supposed to be in. If we declare them in the
Emacs code‐base then the linker, reasonably, expects the functions to
be in the Emacs code‐base.

The top answer here might explain it better:

https://stackoverflow.com/questions/274753/how-to-make-weak-linking-work-with-gcc

I suppose this means we simply can’t guarantee perfect forward
compatibility. It’s a shame, but I don’t see a way round it.

> If they look okay, could you please integrate these changes into your
> patch?  Thanks a lot for your help on this.

If you can confirm the scrollerWidthForControlSize thing, I’ll
incorporate everything else.

Thank you for your help.

Oh, by the way, are you able to check whether the .app built on 10.6
actually runs on something higher?
--
Alan Third



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Charles A. Roelli
On 06/08/2017 23:29, Alan Third wrote:

>> New scroll styles in 10.7+:
>>
>> +enum {
>> +    NSScrollerStyleLegacy = 0,
>> +    NSScrollerStyleOverlay = 1
>> +};
>> +typedef NSInteger NSScrollerStyle;
> I believe we can make this slightly neater:
>
>      enum NSScrollerStyle {
>        NSScrollerStyleLegacy = 0,
>        NSScrollerStyleOverlay = 1
>      };

Strange, it doesn't work here:

 > nsterm.h:58: error: expected ‘)’ before ‘NSScrollerStyle’

>> Add-on to the class declaration of NSScroller (not sure if this is the
>> right way to do it).  Otherwise the compiler errors out on compiling
>> the 10.7+ call to scrollerWidthForControlSize:
>>
>> +@interface NSScroller(NSObject)
>> ++ (CGFloat)scrollerWidthForControlSize:(NSControlSize)controlSize
>> scrollerStyle:(NSScrollerStyle)scrollerStyle;
>> +@end
> Strange... Are you sure this one was an error and not just a warning?
> We’re expecting warnings for this type of thing.

Without it, I get this:

 > nsterm.m: In function ‘+[EmacsScroller scrollerWidth]’:
 > nsterm.m:8387: warning: ‘NSScroller’ may not respond to
‘+scrollerWidthForControlSize:scrollerStyle:’
 > nsterm.m:8387: error: incompatible types in assignment

I think the compiler has to verify somehow that the result of the call
is a CGFloat.
Casting to CGFloat gives:

 > nsterm.m:8387: error: pointer value used where a floating point value
was expected

>> Forward declarations for functions used by macfont.m (declared as weak
>> imports since they won't all be available unless we're on 10.8+):
>>
>> +void CTFontDrawGlyphs(CTFontRef font, const CGGlyph *glyphs, const CGPoint
>> *positions, size_t count, CGContextRef context)
>> __attribute__((weak_import));
>> +#endif
>> +
>> +#if !defined (MAC_OS_X_VERSION_10_8) && MAC_OS_X_VERSION_MAX_ALLOWED >=
>> 1080
>> +extern CFArrayRef CTFontCopyDefaultCascadeListForLanguages(CTFontRef font,
>> CFArrayRef languagePrefList) __attribute__((weak_import));
>>
>> The compiler issued no complaints here, but the linker would not link
>> temacs unless the symbols were listed as permitted to be undefined,
>> using this in src/Makefile:
>>
>> ## System-specific LDFLAGS.
>> LD_SWITCH_SYSTEM= -Wl,-U,_CTFontCopyDefaultCascadeListForLanguages
>> -Wl,-U,_CTFontDrawGlyphs
> I’ve done a bit more reading up on this and I think I’ve misunderstood
> how this works, and probably mislead you.
>
> It seems these functions need to be declared as weak in the definition
> of the library they’re supposed to be in. If we declare them in the
> Emacs code‐base then the linker, reasonably, expects the functions to
> be in the Emacs code‐base.

Maybe I'm also confused.  I thought we would be able to do this,
since:

   - At link time, the symbol is marked as a weak reference, to be
     resolved at runtime.

   - At runtime, the dynamic linker resolves the reference to the weak
     symbol, setting it to NULL if it isn't available.  Normally the
     definition of the function will be found in a dynamic library that
     is part of macOS (as far as I understand).

The Apple compiler/linker should be capable of doing this, supposedly,
as long as you give the magical -Wl,-U,_symbol command line arguments
to the linker.  See also https://stackoverflow.com/a/34983229.


>> If they look okay, could you please integrate these changes into your
>> patch?  Thanks a lot for your help on this.
> If you can confirm the scrollerWidthForControlSize thing, I’ll
> incorporate everything else.
>
> Thank you for your help.
>
> Oh, by the way, are you able to check whether the .app built on 10.6
> actually runs on something higher?

I'd like to check, but wouldn't I need to either:

a) Statically link libraries Emacs depends on, or
b) Include the dependent libraries in the app bundle?

By the libraries Emacs depends on, I mean the ones starting with $HOME here:

$ otool -L src/emacs
src/emacs:
     /System/Library/Frameworks/AppKit.framework/Versions/C/AppKit
(compatibility version 45.0.0, current version 1038.36.0)
     /System/Library/Frameworks/IOKit.framework/Versions/A/IOKit
(compatibility version 1.0.0, current version 275.0.0)
     $HOME/Build/SnowLeopard/lib/libjpeg.9.dylib (compatibility version
12.0.0, current version 12.0.0)
     $HOME/Build/SnowLeopard/lib/librsvg-2.2.dylib (compatibility
version 43.0.0, current version 43.16.0)
     /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current
version 125.2.11)
     $HOME/Build/SnowLeopard/lib/libgio-2.0.0.dylib (compatibility
version 4503.0.0, current version 4503.0.0)
     $HOME/Build/SnowLeopard/lib/libgdk_pixbuf-2.0.0.dylib
(compatibility version 3401.0.0, current version 3401.0.0)
     $HOME/Build/SnowLeopard/lib/libgobject-2.0.0.dylib (compatibility
version 4503.0.0, current version 4503.0.0)
     $HOME/Build/SnowLeopard/lib/libglib-2.0.0.dylib (compatibility
version 4503.0.0, current version 4503.0.0)
     $HOME/Build/SnowLeopard/lib/libintl.9.dylib (compatibility version
11.0.0, current version 11.4.0)
     $HOME/Build/SnowLeopard/lib/libcairo.2.dylib (compatibility version
11403.0.0, current version 11403.6.0)
     $HOME/Build/SnowLeopard/lib/libxml2.2.dylib (compatibility version
12.0.0, current version 12.4.0)
     /usr/lib/libncurses.5.4.dylib (compatibility version 5.4.0, current
version 5.4.0)
     $HOME/Build/SnowLeopard/lib/libgnutls.30.dylib (compatibility
version 40.0.0, current version 40.0.0)
     /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version
1.2.3)
     /usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current
version 227.0.0)
/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
(compatibility version 150.0.0, current version 550.44.0)
/System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices
(compatibility version 1.0.0, current version 38.0.0)
/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation
(compatibility version 300.0.0, current version 751.63.0)

Do either of these sound feasible?



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Alan Third
On Mon, Aug 07, 2017 at 09:23:10PM +0200, Charles A. Roelli wrote:
> On 06/08/2017 23:29, Alan Third wrote:
> > I believe we can make this slightly neater:
> >
> >      enum NSScrollerStyle {
> >        NSScrollerStyleLegacy = 0,
> >        NSScrollerStyleOverlay = 1
> >      };
>
> Strange, it doesn't work here:

We’ll just go with what works, then.

> > I’ve done a bit more reading up on this and I think I’ve misunderstood
> > how this works, and probably mislead you.
> >
> > It seems these functions need to be declared as weak in the definition
> > of the library they’re supposed to be in. If we declare them in the
> > Emacs code‐base then the linker, reasonably, expects the functions to
> > be in the Emacs code‐base.
>
> Maybe I'm also confused.  I thought we would be able to do this,
> since:
>
>   - At link time, the symbol is marked as a weak reference, to be
>     resolved at runtime.
>
>   - At runtime, the dynamic linker resolves the reference to the weak
>     symbol, setting it to NULL if it isn't available.  Normally the
>     definition of the function will be found in a dynamic library that
>     is part of macOS (as far as I understand).
>
> The Apple compiler/linker should be capable of doing this, supposedly,
> as long as you give the magical -Wl,-U,_symbol command line arguments
> to the linker.  See also https://stackoverflow.com/a/34983229.
That’s quite a good description. I guess that we want to do what
you’re suggesting, then. I’m not sure how, though. I’ll try to have a
look through configure.ac to see if I can work it out sometime over
the weekend.

> I'd like to check, but wouldn't I need to either:
>
> a) Statically link libraries Emacs depends on, or
> b) Include the dependent libraries in the app bundle?

Yes, I suppose so. I kind of assumed it would statically link at least
some of them, but I guess not.

I’ve had a look at the build scripts for emacsformacosx.com, but I
don’t understand what they’re doing.

I’ve attached what I have so far, which I think includes all your
changes except for the requirements for linker arguments.
Unfortunately master doesn’t build here now because of some other
problem so it’s untested.
--
Alan Third

0001-Allow-use-of-run-time-OS-version-checks-on-macOS-bug.patch (27K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Charles A. Roelli
Hm, on second thoughts, it seems a bit overwrought to try doing this
weak linking only for the sake of forward-compatible builds.  It
should be enough to support only backward-compatible builds, so that
we might one day distribute Emacs as a .dmg (built on the latest macOS
and backwards-compatible with the oldest version of macOS that we
support).  I don't want to waste time adding code that will hardly
ever be run, so we can take out those forward declarations.  Sorry for
the trouble!  I will send a patch on top of your new one that removes
them (I'm also having build trouble at the moment).

I also looked at the emacsformacosx.com build scripts, and it seems
like they're making copies of Emacs' dependent dynamic libraries,
including them in the application bundle, then using
install_name_tool(1) to patch the Emacs binary to depend on them (I
don't understand, though, how those scripts resolve dependencies
between the dynamic libraries themselves).


On 10/08/2017 23:04, Alan Third wrote:

> On Mon, Aug 07, 2017 at 09:23:10PM +0200, Charles A. Roelli wrote:
>> On 06/08/2017 23:29, Alan Third wrote:
>>> I believe we can make this slightly neater:
>>>
>>>       enum NSScrollerStyle {
>>>         NSScrollerStyleLegacy = 0,
>>>         NSScrollerStyleOverlay = 1
>>>       };
>> Strange, it doesn't work here:
> We’ll just go with what works, then.
>
>>> I’ve done a bit more reading up on this and I think I’ve misunderstood
>>> how this works, and probably mislead you.
>>>
>>> It seems these functions need to be declared as weak in the definition
>>> of the library they’re supposed to be in. If we declare them in the
>>> Emacs code‐base then the linker, reasonably, expects the functions to
>>> be in the Emacs code‐base.
>> Maybe I'm also confused.  I thought we would be able to do this,
>> since:
>>
>>    - At link time, the symbol is marked as a weak reference, to be
>>      resolved at runtime.
>>
>>    - At runtime, the dynamic linker resolves the reference to the weak
>>      symbol, setting it to NULL if it isn't available.  Normally the
>>      definition of the function will be found in a dynamic library that
>>      is part of macOS (as far as I understand).
>>
>> The Apple compiler/linker should be capable of doing this, supposedly,
>> as long as you give the magical -Wl,-U,_symbol command line arguments
>> to the linker.  See also https://stackoverflow.com/a/34983229.
> That’s quite a good description. I guess that we want to do what
> you’re suggesting, then. I’m not sure how, though. I’ll try to have a
> look through configure.ac to see if I can work it out sometime over
> the weekend.
>
>> I'd like to check, but wouldn't I need to either:
>>
>> a) Statically link libraries Emacs depends on, or
>> b) Include the dependent libraries in the app bundle?
> Yes, I suppose so. I kind of assumed it would statically link at least
> some of them, but I guess not.
>
> I’ve had a look at the build scripts for emacsformacosx.com, but I
> don’t understand what they’re doing.
>
> I’ve attached what I have so far, which I think includes all your
> changes except for the requirements for linker arguments.
> Unfortunately master doesn’t build here now because of some other
> problem so it’s untested.




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Charles A. Roelli
(I fixed the build issue with 'make bootstrap')

Attached is the patch to revert my changes.


On 12/08/2017 13:13, Charles A. Roelli wrote:

> Hm, on second thoughts, it seems a bit overwrought to try doing this
> weak linking only for the sake of forward-compatible builds.  It
> should be enough to support only backward-compatible builds, so that
> we might one day distribute Emacs as a .dmg (built on the latest macOS
> and backwards-compatible with the oldest version of macOS that we
> support).  I don't want to waste time adding code that will hardly
> ever be run, so we can take out those forward declarations.  Sorry for
> the trouble!  I will send a patch on top of your new one that removes
> them (I'm also having build trouble at the moment).
>
> I also looked at the emacsformacosx.com build scripts, and it seems
> like they're making copies of Emacs' dependent dynamic libraries,
> including them in the application bundle, then using
> install_name_tool(1) to patch the Emacs binary to depend on them (I
> don't understand, though, how those scripts resolve dependencies
> between the dynamic libraries themselves).
>
>
> On 10/08/2017 23:04, Alan Third wrote:
>> On Mon, Aug 07, 2017 at 09:23:10PM +0200, Charles A. Roelli wrote:
>>> On 06/08/2017 23:29, Alan Third wrote:
>>>> I believe we can make this slightly neater:
>>>>
>>>>       enum NSScrollerStyle {
>>>>         NSScrollerStyleLegacy = 0,
>>>>         NSScrollerStyleOverlay = 1
>>>>       };
>>> Strange, it doesn't work here:
>> We’ll just go with what works, then.
>>
>>>> I’ve done a bit more reading up on this and I think I’ve misunderstood
>>>> how this works, and probably mislead you.
>>>>
>>>> It seems these functions need to be declared as weak in the definition
>>>> of the library they’re supposed to be in. If we declare them in the
>>>> Emacs code‐base then the linker, reasonably, expects the functions to
>>>> be in the Emacs code‐base.
>>> Maybe I'm also confused.  I thought we would be able to do this,
>>> since:
>>>
>>>    - At link time, the symbol is marked as a weak reference, to be
>>>      resolved at runtime.
>>>
>>>    - At runtime, the dynamic linker resolves the reference to the weak
>>>      symbol, setting it to NULL if it isn't available. Normally the
>>>      definition of the function will be found in a dynamic library that
>>>      is part of macOS (as far as I understand).
>>>
>>> The Apple compiler/linker should be capable of doing this, supposedly,
>>> as long as you give the magical -Wl,-U,_symbol command line arguments
>>> to the linker.  See also https://stackoverflow.com/a/34983229.
>> That’s quite a good description. I guess that we want to do what
>> you’re suggesting, then. I’m not sure how, though. I’ll try to have a
>> look through configure.ac to see if I can work it out sometime over
>> the weekend.
>>
>>> I'd like to check, but wouldn't I need to either:
>>>
>>> a) Statically link libraries Emacs depends on, or
>>> b) Include the dependent libraries in the app bundle?
>> Yes, I suppose so. I kind of assumed it would statically link at least
>> some of them, but I guess not.
>>
>> I’ve had a look at the build scripts for emacsformacosx.com, but I
>> don’t understand what they’re doing.
>>
>> I’ve attached what I have so far, which I think includes all your
>> changes except for the requirements for linker arguments.
>> Unfortunately master doesn’t build here now because of some other
>> problem so it’s untested.
>


remove-forward-compatibility-changes.diff (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Alan Third
In reply to this post by Charles A. Roelli
On Sat, Aug 12, 2017 at 01:13:56PM +0200, Charles A. Roelli wrote:
> Hm, on second thoughts, it seems a bit overwrought to try doing this
> weak linking only for the sake of forward-compatible builds.  It
> should be enough to support only backward-compatible builds, so that
> we might one day distribute Emacs as a .dmg (built on the latest macOS
> and backwards-compatible with the oldest version of macOS that we
> support).

I think this makes sense. Especially given we’re not able to actually
create a stand‐alone .app without implementing something like...

> I also looked at the emacsformacosx.com build scripts, and it seems
> like they're making copies of Emacs' dependent dynamic libraries,
> including them in the application bundle, then using
> install_name_tool(1) to patch the Emacs binary to depend on them (I
> don't understand, though, how those scripts resolve dependencies
> between the dynamic libraries themselves).

I wouldn’t have a problem with putting this capability in, but I don’t
have the knowledge nor the inclination to do it myself.

I was going to write something up in INSTALL about building with
feature detection, but I really don’t know how to put it. I don’t want
to give the impression that if you use
-DMAC_OS_X_VERSION_MIN_ALLOWED=1060 that it will magically build a
portable .app. I began to wonder if it’s worth mentioning at all since
I doubt any more than a handful of people will be interested in
building with this option.

David and David, I hope it’s OK to include you in this. I thought you
might be interested and perhaps have some thoughts on what we’re
doing.

Basically, instead of detecting all macOS features at compile‐time,
we’ve stuck in some code to detect them at run‐time. It causes
compiler warnings, so by default it still limits features to those
available at build‐time, but if you do something like:

    ./configure --with-ns CFLAGS="-DMAC_OS_X_VERSION_MIN_REQUIRED=1060 -O3 -g"

when building on macOS Sierra, you should, in theory, end up with an
executable that will work correctly on every version of macOS back to
10.6, inclusive. We haven’t been able to properly test portability yet
as it requires including dynamic libraries.

The patch is attached to this email.
--
Alan Third

0001-Allow-use-of-run-time-OS-version-checks-on-macOS-bug.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27810: NS runtime feature detection

Alan Third
In reply to this post by Charles A. Roelli
On Sat, Aug 12, 2017 at 03:02:07PM +0200, Charles A. Roelli wrote:
> Attached is the patch to revert my changes.

I’ve pushed it to master now.
--
Alan Third



Loading...