Bug 30526

Summary: Supporting multiple OS X versions in one binary
Product: WebKit Reporter: Kevin Ollivier <kevino>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: ahmad.saleem792, ap, bfulgham, mitz, mrowe
Priority: P2 Keywords: Wx
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Patch to allow compilation using 10.5 SDK on SnowLeopard
mrowe: review-
Changes to enable a Tiger-SnowLeopard compatible binary for non-Apple ports none

Description Kevin Ollivier 2009-10-19 12:27:16 PDT
While playing around with adding the ability to specify the SDK to build against to wxWebKit, I discovered something interesting. With one simple change to Platform.h, I was able to get a working wxWebKit Mac binary that worked on both 10.5 and 10.6. (Basically just disabling some 10.6 specific code when using an older SDK to build on SnowLeopard.) Of course, while compilation works for 10.4 as well, it does not work with memory allocation errors. Still, even 10.5 + 10.6 support in one binary would be a huge gain for us developers that aren't able to bundle our software with the OS. :)

I'm enclosing the patch of the change I made for discussion. I'm not sure it's really the 'right' fix, though I'm so far unable to crash it on either OS even after loading JS heavy pages. I'd like to start a discussion of what else might need to be done for 10.5 + 10.6 support to really be solid, and also start to look at 10.4 compat issues. Is compiling 10.4 code going to actually break support for 10.5+ (e.g. by changing BUILDING_ON_TIGER to return MIN_REQUIRED rather than MAX_ALLOWED, or perhaps to introduce a BUILDING_TIGER_COMPAT define), or will there just be a degradation of features as we fallback to 10.4 compatible approaches? Also, what are the specific areas of the code I should be paying particular attention to when working out a compatibility solution?

Thanks,

Kevin
Comment 1 Kevin Ollivier 2009-10-19 12:35:57 PDT
Created attachment 41439 [details]
Patch to allow compilation using 10.5 SDK on SnowLeopard

As I said, originally I was just trying to compile by using MACOSX_DEPLOYMENT_TARGET=10.5 and specifying the 10.5 SDK but as far as I can tell, this patch creates a binary that works fine on 10.6 as well.
Comment 2 Mark Rowe (bdash) 2009-10-19 13:06:34 PDT
Comment on attachment 41439 [details]
Patch to allow compilation using 10.5 SDK on SnowLeopard

It may appear to work but it will have subtle bugs as *all* of the code wrapped with these macros was written with the assumption that the code within these blocks was targeted for the specific OS version.  When building in the manner you mention it will be running on a different version of the OS and may not behave as expected.  A number of aspects of the code would likely need to be updated to determine the underlying OS version at runtime and to react accordingly.

It also doesn’t make sense to switch a single use of BUILDING_ON_TIGER and BUILDING_ON_LEOPARD to using availability macros.  If we’re going to make this change it needs to be done in a more consistent manner.
Comment 3 Kevin Ollivier 2009-10-19 14:35:42 PDT
(In reply to comment #2)
> (From update of attachment 41439 [details])
> It may appear to work but it will have subtle bugs as *all* of the code wrapped
> with these macros was written with the assumption that the code within these
> blocks was targeted for the specific OS version.  When building in the manner
> you mention it will be running on a different version of the OS and may not
> behave as expected.  A number of aspects of the code would likely need to be
> updated to determine the underlying OS version at runtime and to react
> accordingly.
> 
> It also doesn’t make sense to switch a single use of BUILDING_ON_TIGER and
> BUILDING_ON_LEOPARD to using availability macros.  If we’re going to make this
> change it needs to be done in a more consistent manner.

Okay, after doing a grep what I'm seeing is that a vast majority of the use of
these macros are in Mac port sources or code blocks, which won't use this
support anyway, at least not in the short term. Perhaps that somewhat explains
why the change didn't seem so problematic for the wx port.

So, how about this? We create a SUPPORTS_<CAT> or <CAT>_COMPATIBLE (where CAT
is release name) type of define that corresponds to MIN_REQUIRED for non-Mac
ports but for Mac (or any port that wants to keep the existing behavior) will
simply be defined to  BUILDING_ON_<CAT> for compatibility. The only other thing
this would probably require is to add weakly defined symbol checks to allow to
use the newer API when running on a newer release, but since wx doesn't use
much of the Mac port's Cocoa, CF or CG code,  nor presumably would Qt or GTK, I
suspect this would actually only need to be done in a few spots.

So, does this sound reasonable? If not, any ideas on how else we could approach
this issue?

Thanks,

Kevin
Comment 4 Mark Rowe (bdash) 2009-10-19 14:55:42 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 41439 [details] [details])
> > It may appear to work but it will have subtle bugs as *all* of the code wrapped
> > with these macros was written with the assumption that the code within these
> > blocks was targeted for the specific OS version.  When building in the manner
> > you mention it will be running on a different version of the OS and may not
> > behave as expected.  A number of aspects of the code would likely need to be
> > updated to determine the underlying OS version at runtime and to react
> > accordingly.
> > 
> > It also doesn’t make sense to switch a single use of BUILDING_ON_TIGER and
> > BUILDING_ON_LEOPARD to using availability macros.  If we’re going to make this
> > change it needs to be done in a more consistent manner.
> 
> Okay, after doing a grep what I'm seeing is that a vast majority of the use of
> these macros are in Mac port sources or code blocks, which won't use this
> support anyway, at least not in the short term. Perhaps that somewhat explains
> why the change didn't seem so problematic for the wx port.

Many of them are Mac-only, yes.  ICU is a significant case that is not specific to the Mac version of WebKit, but where the behavior can differ significantly from Tiger’s version to SnowLeopard.  We handle this with compile-time checks.

> So, how about this? We create a SUPPORTS_<CAT> or <CAT>_COMPATIBLE (where CAT
> is release name) type of define that corresponds to MIN_REQUIRED for non-Mac
> ports but for Mac (or any port that wants to keep the existing behavior) will
> simply be defined to  BUILDING_ON_<CAT> for compatibility. The only other thing
> this would probably require is to add weakly defined symbol checks to allow to
> use the newer API when running on a newer release, but since wx doesn't use
> much of the Mac port's Cocoa, CF or CG code,  nor presumably would Qt or GTK, I
> suspect this would actually only need to be done in a few spots.

I can’t easily follow what you’re suggesting here.
Comment 5 Kevin Ollivier 2009-10-19 23:41:55 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 41439 [details] [details] [details])
> > > It may appear to work but it will have subtle bugs as *all* of the code wrapped
> > > with these macros was written with the assumption that the code within these
> > > blocks was targeted for the specific OS version.  When building in the manner
> > > you mention it will be running on a different version of the OS and may not
> > > behave as expected.  A number of aspects of the code would likely need to be
> > > updated to determine the underlying OS version at runtime and to react
> > > accordingly.
> > > 
> > > It also doesn’t make sense to switch a single use of BUILDING_ON_TIGER and
> > > BUILDING_ON_LEOPARD to using availability macros.  If we’re going to make this
> > > change it needs to be done in a more consistent manner.
> > 
> > Okay, after doing a grep what I'm seeing is that a vast majority of the use of
> > these macros are in Mac port sources or code blocks, which won't use this
> > support anyway, at least not in the short term. Perhaps that somewhat explains
> > why the change didn't seem so problematic for the wx port.
> 
> Many of them are Mac-only, yes.  ICU is a significant case that is not specific
> to the Mac version of WebKit, but where the behavior can differ significantly
> from Tiger’s version to SnowLeopard.  We handle this with compile-time checks.
> 
> > So, how about this? We create a SUPPORTS_<CAT> or <CAT>_COMPATIBLE (where CAT
> > is release name) type of define that corresponds to MIN_REQUIRED for non-Mac
> > ports but for Mac (or any port that wants to keep the existing behavior) will
> > simply be defined to  BUILDING_ON_<CAT> for compatibility. The only other thing
> > this would probably require is to add weakly defined symbol checks to allow to
> > use the newer API when running on a newer release, but since wx doesn't use
> > much of the Mac port's Cocoa, CF or CG code,  nor presumably would Qt or GTK, I
> > suspect this would actually only need to be done in a few spots.
> 
> I can’t easily follow what you’re suggesting here.

I'm basically suggesting we add defines such as SUPPORTS_TIGER which would be set as follows

#if PLATFORM(MAC)
#if BUILDING_ON_TIGER
#define SUPPORTS_TIGER // for Apple port, SUPPORTS_TIGER == BUILDING_ON_TIGER
#endif
#elif PLATFORM(DARWIN) && defined(MAC_OS_X_VERSION_10_4)
                && MAC_OS_X_MIN_REQUIRED == MAC_OS_X_VERSION_10_4
#define SUPPORTS_TIGER // can be true even under Leopard or Snow Leopard
#endif

Then say we have the following example code block somewhere in common code:

#if BUILDING_ON_TIGER
... 10.4 compatible code ...
#elif BUILDING_ON_LEOPARD
... 10.5 compatible code ...
#else
... 10.6 compatible code ...
#endif

It would change to:

#if SUPPORTS_TIGER
... 10.4 compatible code ...
#elif SUPPORTS_LEOPARD
... 10.5 compatible code ...
#else
... 10.6 compatible code ...
#endif

So the Apple/Mac port would compile this code exactly the same as it does today, because there SUPPORTS_TIGER is only true when BUILDING_ON_TIGER is true, but ports like wx would compile the 10.4 codepath even under 10.5 or 10.6 so long as 10.4 support was set as the min required system. Does this make things clearer?
Comment 6 Mark Rowe (bdash) 2009-10-19 23:47:25 PDT
And how does it address the case where you need to do different things based on the OS version?  It seems as though you’d need to duplicate code.
Comment 7 Kevin Ollivier 2009-10-21 13:07:22 PDT
Created attachment 41603 [details]
Changes to enable a Tiger-SnowLeopard compatible binary for non-Apple ports

I thought it would be easier to produce a patch to make it easier to see the actual code in question, so that we could discuss the cases that need changed in detail. I'm working on setting up a Tiger environment to start testing the binary created by this patch, but I've checked the defines numerous times and to the best of my knowledge this patch does change every define that needs to be changed for a non-Apple port to have multi-OS version compatibility. I didn't mark it for review since it's not polished yet as I may need to make more changes after running some tests on Tiger.

Outside of switching defines, so far I found two places where we would need (or it would be desirable to have) dynamic version checks rather than compile-time checks. One is for a Leopard performance improvement in FrameLoader.cpp, and the other is for the ICU 3.2 code in RenderText.cpp, which I've implemented. The only other cases I could see that might benefit from some sort of dynamic check would be the USE_BACKGROUND_THREAD_TO_SCAVENGE_MEMORY  codepath in FastMalloc.cpp (the note suggests we can compile the code on Tiger), and the spelling and grammar changes in Editor.cpp and ContextMenu.cpp, though I wonder if other ports like Qt and GTK would actually using OS X's spelling and grammar support, as with wx we would need to solve the problem for other environments anyway, and if we went that far, we could just use the solution on Mac as well and get Tiger support in the mix.

So, any thoughts/preferences on the above issues? Do you see any particular issues with this patch?
Comment 8 Ahmad Saleem 2022-08-29 03:16:19 PDT
ap@webkit.org - Is this needed anymore, it has Keyword "Wx" but does not seem to be limited to it. Appreciate if you can mark this accordingly. Thanks!
Comment 9 Alexey Proskuryakov 2022-08-29 08:38:28 PDT
We still don't support the use case of embedding one's own copy of Cocoa WebKit in applications, and the wx port is gone.

Other ports like Gtk may or may not need this. If they do, tracking with separate bugs would be more appropriate than reviving this wx bug.