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

 
 

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

Submitter:  Andrew Janke <apjanke>
Submitted:  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
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group administrator
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>
Group 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>
Group administrator
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>
Group administrator
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>
Group administrator
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>
Group Member
Thu 06 Feb 2020 03:46:22 PM UTC, comment #1: 

@Andrew, the patch works for me

Ben Abbott <bpabbott>
Group 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)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group 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.13-758e.
    Corresponding source code