bugGNU Octave - Bugs: bug #57754, Back out App Nap objc_msgSend fix...

 
 

bug #57754: Back out App Nap objc_msgSend fix on Mojave+

Submitted by:  Andrew Janke <apjanke>
Submitted on:  Thu 06 Feb 2020 10:52:48 AM UTC  
 
Category:  Configuration and Build System Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Build Failure
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  Mac OS

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Sun 16 Feb 2020 09:17:55 AM UTC, comment #10: 

Ben and Andrew, thanks for testing.
I pushed the patch here:
http://hg.savannah.gnu.org/hgweb/octave/rev/60e4a9909fac

Closing as fixed.

Markus Mützel <mmuetzel>
Project Member
Sat 15 Feb 2020 08:12:14 PM UTC, comment #9: 

Works for me on macOS 10.13 and 10.14. I think this is the Real Fix I was hoping for!

Andrew Janke <apjanke>
Sat 15 Feb 2020 06:53:57 PM UTC, comment #8: 

@Markus

Your patch works for me (macOS 10.15.3)!

Ben Abbott <bpabbott>
Project Member
Fri 14 Feb 2020 05:35:11 PM UTC, comment #7: 

Inadvertently pasted something on the wrong line. Next (and hopefully final) try.

(file #48410)

Markus Mützel <mmuetzel>
Project Member
Fri 14 Feb 2020 05:29:08 PM UTC, comment #6: 

Forgot to also change the 4th call of "objc_msgSend". Should be fixed in the attached patch.

(file #48409)

Markus Mützel <mmuetzel>
Project Member
Fri 14 Feb 2020 03:02:12 PM UTC, comment #5: 

I don't have a Mac to test on. But could the following change make it work for the old and the new prototype of "objc_msgSend"?

diff -r 8f5e625062f3 libgui/src/octave-qobject.cc
--- a/libgui/src/octave-qobject.cc        Thu Feb 13 11:33:27 2020 -0500
+++ b/libgui/src/octave-qobject.cc        Fri Feb 14 16:00:19 2020 +0100
@@ -88,16 +88,18 @@
       == nullptr)
     return;

-  process_info = objc_msgSend (reinterpret_cast<id> (process_info_class),
-                               process_info_selector);
+  process_info = reinterpret_cast<id (*)(id, SEL)> (objc_msgSend)
+                   (reinterpret_cast<id> (process_info_class),
+                    process_info_selector);
   if (process_info == nil)
     return;

-  reason_string = objc_msgSend (reinterpret_cast<id> (objc_getClass ("NSString")),
-                                sel_getUid ("alloc"));
-  reason_string = objc_msgSend (reason_string,
-                                sel_getUid ("initWithUTF8String:"),
-                                "App Nap causes pause() malfunction");
+  reason_string = reinterpret_cast<id (*)(id, SEL)> (objc_msgSend)
+                    (reinterpret_cast<id> (objc_getClass ("NSString")),
+                     sel_getUid ("alloc"));
+  reason_string = reinterpret_cast<id (*)(id, SEL, const char *)> (objc_msgSend)
+                    (reason_string, sel_getUid ("initWithUTF8String:"),
+                     "App Nap causes pause() malfunction");

   // Start an Activity that suppresses App Nap.  This Activity will run for
   // the entire duration of the Octave process.  This is intentional,

Markus Mützel <mmuetzel>
Project Member
Wed 12 Feb 2020 10:29:59 AM UTC, comment #4: 

Here's a patch to do the disabling using a behavior test in configure instead of checking the macOS version: https://savannah.gnu.org/bugs/download.php?file_id=48394

Andrew Janke <apjanke>
Fri 07 Feb 2020 12:33:21 AM UTC, comment #3: 

Yeah, a configure compile test should be pretty straightforward. I'll give it a try this weekend.

Andrew Janke <apjanke>
Fri 07 Feb 2020 12:21:01 AM UTC, comment #2: 

I think the conditional would be better written as a single line as

#if defined (__MAC_OS_X_VERSION_MAX_ALLOWED) && __MAC_OS_X_VERSION_MAX_ALLOWED < 101400

to generally agree with our preprocessor code style (grep for "#.*if.*defined.*[<>]").

We also generally prefer behavior tests to version comparisons, is it possible to write a configure compile test that would fail on systems where this no longer works?

Mike Miller <mtmiller>
Project Administrator
Thu 06 Feb 2020 03:46:22 PM UTC, comment #1: 

@Andrew, the patch works for me

Ben Abbott <bpabbott>
Project Member
Thu 06 Feb 2020 10:52:48 AM UTC, original submission:  

Hi folks,

So, that App Nap "pause takes too long" fix I submitted a while back? Looks like it breaks the build on newer versions of macOS.

Ben Abbott reports on the Octave Maintainers mailing list that the build is broken for him on macOS 10.15 Catalina. https://lists.gnu.org/archive/html/octave-maintainers/2020-02/msg00005.html

Surprisingly, the build is also broken for me on my macOS 10.14 Mojave box, for this objc_msgSend reason, where I think it used to be working. I can also reproduce Ben's build failure in a 10.15 Catalina VM.

I suggest we just "#if" it out, and live with the performance hit, so that we can still build on current macOS version.

Patch attached.

Andrew Janke <apjanke>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #48410:  bug57754_AppNap_Mojave_v2.patch added by mmuetzel (2KiB - application/octet-stream)
file #48409:  bug57754_AppNap_Mojave.patch added by mmuetzel (2KiB - application/octet-stream)
file #48360:  nevermind-about-appnap.patch added by apjanke (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by bpabbott (Posted a comment)
  • -email is unavailable- added by apjanke (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-02-16 mmuetzel StatusWorks For Me => Fixed
        Open/ClosedOpen => Closed
    2020-02-15 bpabbott StatusPatch Submitted => Works For Me
    2020-02-14 mmuetzel Attached File- => Added bug57754_AppNap_Mojave_v2.patch, #48410
        StatusIn Progress => Patch Submitted
    2020-02-14 mmuetzel Attached File- => Added bug57754_AppNap_Mojave.patch, #48409
    2020-02-12 apjanke Attached File- => Added disable-appnap-fix-using-configure-test.patch, #48394
    2020-02-07 rik5 StatusNone => In Progress
        Release5.1.0 => dev
    2020-02-06 apjanke Attached File- => Added nevermind-about-appnap.patch, #48360

    Back to the top


    Powered by Savane 3.5