Rawtherapee: Update MacPorts instructions and macOS bundle scripts

[ion12]

Update macports-based compiling instructions:

macports variants:
+quartz -x11
(+no_gnome +no_x11 are obsolete)

GTK3 dependencies:
gtk3 gtkmm3 gtk-osx-application-gtk3 adwaita-icon-theme libsigcxx2 lcms2 libiptcdata fftw-3-single clang-3.9
(openmp is now fully integrated into clang and doesn't need any explicit setting-up)

sudo port install wget
wget is a requirement in the current instructions for the libiconv patch, maybe it should be mentioned in the instructions

Update macosx_bundle.sh for gtk3:

I took the https://filebin.net/kdlrguptt56syza0 patch helmi03 linked to on issue #3339 and tweaked it a bit further to remove all obsolete references to RT4 and gtk2 from macosx_bundle.sh and executable_loader.in + some cosmetic changes that you can ignore if you don't like.
I also added a hard-coded fix to include the 16x16 Adwaita icons + index.theme
I can't find any reason to include the full set of Adwaita icons - but If anyone has a more elegant solution to include Adwaita or if I'm missing any icons please improve/update.
I should mention that I don't have a Retina display, I don't know if that requires including higher resolution icons (?)

I would like to ask about partha's build: do we really need the entire (46MB !!!) folder of Adwaita icons? seems like a shame to bloat the size of the app bundle if they are not all necessary, the build I produce through the macports toolchain comes out significantly smaller and I think it's mostly to do with these icons.

attaching the patch and a build (I'm still on 10.11 so I don't know if it will work on 10.12 as well).

cheers

osx.patch.zip
https://filebin.net/oauqratd8i2i56p9

[heckflosse]

@ion12 '(openmp is now fully integrated into clang and doesn't need any explicit setting-up)'

Does that mean we should review this part of rt-code:

https://github.com/Beep6581/RawTherapee/blob/dev/rtengine/opthelper.h#L72

Edit: Can you try a build without lines 72 and 74 of opthelper.h?

Ingo

[heckflosse]

@ion12 I ask, because in past (using older versions of clang) using nested omp the way we do in rt at some places led to clang crashes (not rt crashes, but crashing the clang build process). In case we can remove this conditions now, that will lead to faster noise reduction in rt clang builds

Ingo

[gaaned92]

@ion12

I would like to ask about partha's build: do we really need the entire (46MB !!!) folder of Adwaita icons?

Following Sguyader advice, I only bundle for windows builds
Adwaita 16x16 (actions,devices, mimetypes, places, status)
48x48 (devices)
cursors (it avoids warnings, but not sure it is required)
icon-theme.cache and index.theme

[ion12]

@heckflosse
I commented out line 72 and line 74 in opthelper.h and experienced no crashes while building or running RT.

@gaaned92
Thanks. On macOS I haven't noticed any warnings about missing cursors, but I'll check about adding the 48x48 "devices" folder and removing the unnecessary 16x16 folders if that's the suggestion

[heckflosse]

@ion12 Great! Thanks for testing. I will push the modified opthelper.h soon :+1:

[heckflosse]

@ion12 in rtengine/curves.h line 252 there is another condition related to clang. Can you replace __clang__ with __foobar__ in that line and test that. If that works, I will change that too.

[ion12]

build failed with error:

/rtengine/curves.h:253:17: error: value of
      type 'int __attribute__((ext_vector_type(4)))' (vector of 4 'int' values) is not
      contextually convertible to 'bool'

called from
/rtengine/flatcurves.cc:19
/rtengine/curves.cc:35
/rtengine/diagonalcurves.cc:21

[heckflosse]

@ion12 Ok, that means clang still does not support that kind of writing vectorized code. Does not really matter for performance. I will push the changes to opthelper.h now to dev branch. That at least gives a speedup for some parts of rt when clang is used to build.
Thanks a lot for testing :+1:

Ingo

[heckflosse]

@ion12 I pushed the changes for opthelper.h to dev branch.

[heckflosse]

@ion12 for information: #3324 was one of the cases which did not compile in past without the special handling of clang

[ion12]

@heckflosse happy to help test 👍
I remember having several cases of clang/openmp related build stoppers in the past and waiting helplessly for you to fix them ;)

[heckflosse]

@ion12 I could not fix them (only work around them), because they were clang faults. But now it seems to work. Good!

[ion12]

@heckflosse you worked around them, that was as good as a fix.. it was a real headache for a while when Apple first dropped support for gcc

[heckflosse]

@ion12 thanks :)

[TooWaBoo]

@ion12

and removing the unnecessary 16x16

Don't remove this folder!!! This is the most important one.

[ion12]

@TooWaBoo I meant the unnecessary 16x16 subfolders (emblems, emotes and so on).
I updated the patch to match gaaned92/Sguyader folder list suggestion.
osx.patch.zip
also attached is dev branch build if anyone cares to compare
https://filebin.net/e3d3zbo3ah4oc1rx

[ion12]

@Beep6581
Is it possible to update Rawpedia and commit some sort of fix (not necessarily helmi03's or my patch, but something...) for macosx_bundle.sh / executable_loader.in so it would be possible to correctly compile gtk3 RT?
Happy to help in any way I can.
In my patch I commented out copying the gtk-keys.css files because I wasn't sure if they are actually relevant, but probably safer to include them anyway.

There is another small fix needed - libiconv is now on version 1.15, the patch needs to be updated to the correct version, and the changes should now start from line 611 instead of 607, otherwise the patch will fail. Other than that the patch is unchanged.
You can still use the 1.14 patch even if you update macports libiconv to 1.15, and RT build will work, but macports will keep complaining that it has a broken port everytime you update/install anything through it and it's not ideal.

thanks.

[Beep6581]

@ion12 @helmi03 @partha1b you guys seem to be compiling in different ways, using different technologies. I have no say in this and no judgement in what is best/easiest/fool-proof/future-proof in macOS. You are all welcome to sign up for RawPedia accounts and update the macOS article. You would need to reach a consensus on which way to go about it.

[ion12]

@Beep6581
currently the RawPedia instructions target macport as the technology to setup the build toolchain, (which is what I am using), and as long as that's the case, it should be correct and up to date - even if there is a decision to promote other technologies (homebrew, jhbuild) in addition or instead at a later time.
I requested an account and happy to update the current macports instructions myself for now.

As for the macosx_bundle.sh script - this is what is currently being provided in the sources to succesfully build RT on macOS, I don't think it should be left broken even if a more thorugh re-design is decided later. Same goes for the libiconv patch.
These are simple fixes for me locally, but if they are not commited they might be breakers for others not familiar with RT trying to compile.
My patch is just a modification of helmi03's patch, so clearly he is using the same bundle script as well and I can strip my patch down to the absolute bare minimum required to correct for the gtk3 transition, or just use his patch for now.

I've been using Partha's Gimp builds for both Windows and Mac for years (@partha1b : thanks so much for all the effort!) and would consider him much more qualified than I, but I'm thinking he does a lot of customized work to set up his GTK+ build environment (and doesn't use the provided bundle bash script anyway?) and that is probably better for official releases but much less approachable for people who are not dedicated developers or packagers..
I love the fact that it so (relatively) easy for an enthusiastic end-user to compile RT on the Mac, test dev builds, make small customization - almost like it is on gnu-linux. I think it's something worth maintaining.

[Beep6581]

@ion12 your account is confirmed and edit rights granted.

Could you open a PR for the code changes to macosx_bundle.sh et al.?

[Partha1b]

@Beep6581 Sorry for the late response. I have been busy as real life intrudes. 🙂

I am still hoping that @heckflosse is able to fix the bug I identified or to confirm that it's an issue with my setup. Since I've been using my setup (where everything is built from scratch with Apple provided clang) to build McGimp for a long time without any crashes, I have confidence in my setup.

@ion12 Thanks for your kind words! I appreciate it. 😄

[ion12]

@Beep6581 updated 'compiling on macOS' page on RawPedia. Still macports-centric for now, to keep things clear and concise.
opened a PR, it's my first time so hope it's done ok.

@Partha1b In your setup do the Adwaita icons get bundled automatically? Under macports I couldn't figure out a way to do that and I copy them explicitly in the bundle script.
Is it not a good idea to bundle only the Adwaita icons that are absolutely necessary, to avoid bloat?

I would love to learn how you setup your build toolchain (if/when you have the time and if you don't mind sharing), and I'm also curious about the way you structure your .app bundle, as it's different from any other example of unix based .app structure that I've seen so far.

I do agree with @Beep6581 that it would be constructive to decide on macOS standards for bundling/configuration/paths to program files and folders and so on.

[Partha1b]

@ion12 I would be happy to show you how I do my building and packaging.

This is probably not the best venue for that conversation. Why don't we take it offline and you can contact at my email that's on my website?

Thanks!

[ion12]

@Partha1b Thanks for the kind offer and I will take you up on it soon (real life intruding over here a as well right now) :)

When I say 'standard' I don't mean for the whole build setup but it is important for the final configuration of the build product and should probably be resolved here.
For example - I see from another issue that you've moved the user 'config' folder path to 'Application Support', which makes a lot of sense, but RawPedia still assumes it is in the old path and so does the bundle script included in the sources, which makes it confusing - and harder to help out if there are any errors. Basically I'm suggesting you share your packaging settings so we can keep everything in sync :)

If there are other mac users compiling for themselves and reading this, I would also love to hear if there is there a compelling argument to prefer Homebrew over MacPorts and if anyone is using Homebrew to succesfully compile RT

[Partha1b]

@ion12 Sorry I misunderstood you. I thought you asked about

I would love to learn how you setup your build toolchain (if/when you have the time and if you don't mind sharing), and I'm also curious about the way you structure your .app bundle, as it's different from any other example of unix based .app structure that I've seen so far.

I didn't see anything about "standard". I agree that anything related to RT should be discussed here.

In one of my previous posts I had stated that the Mac standard for settings is the Application Support folder and hence I had set the RT_SETTINGS environment variable to $HOME/Library/Application Support/Rawtherapee in the MacOS folder script.

As for the app structure, I am following the Mac app structure. Even if a Mac OS is based on BSD, it does not mean that Mac apps follow Unix structures. In fact, it most certainly does not.

Thanks,
Partha

[ion12]

@Partha1b Sorry for the delayed reply, very hectic week for me.
I am also interested in learning about how you set up your toolchain, and I agree that it's beyond the scope of this conversation.

I do understand the Apple standards. What confuses me is why there are fonts and icons in the Framewords folder, and why the rt binary is placed in the Resources folder, this seems to be neither Apple standard nor unix standard. I'm sure you have a reason behind this, so I wanted to understand - especially if this is going to become the default for RT distribution.

Also, if you move the config folder to ~/Library/Application Support/Rawtherapee shouldn't the cache folder be moved as well while we're at it? put both config and cache as two discrete folders inside ~/Library/Application Support/Rawtherapee ?

Mainly, when I say 'RT standard' I mean that please share/commit the changes you make to the executable loader script and the bundle/packaging script (tools/osx/executable_loader.in and tools/osx/macosx_bundle.sh)
If you feel that they need a major re-vamping then so be it. And if we need to test that they will work for both your toolchain setup and a macports/homebrew based setup, or make some adjustments, I'm happy to help with that.

Hope this explains everything better, many thanks for your improvements so far :)

[Partha1b]

@ion12 I know what you mean. Real life always intrudes. 🙂

I do understand the Apple standards. What confuses me is why there are fonts and icons in the Framewords folder, and why the rt binary is placed in the Resources folder, this seems to be neither Apple standard nor unix standard. I'm sure you have a reason behind this, so I wanted to understand - especially if this is going to become the default for RT distribution.

This is a great question and I am wrestling with it. It's how the cmake file is setup and how RT accesses its assets. Without extensive modification to the cmake files, I could not put RT in the Mac app structure elegantly. So, I put the whole RT folder within the resources folder. Do you have a suggestion for this?

Please note that OSX does put icons in the resources location. See for example https://developer.apple.com/library/content/documentation/CoreFoundation/Conceptual/CFBundles/BundleTypes/BundleTypes.html#//apple_ref/doc/uid/10000123i-CH101-SW1

Also, if you move the config folder to ~/Library/Application Support/Rawtherapee shouldn't the cache folder be moved as well while we're at it? put both config and cache as two discrete folders inside ~/Library/Application Support/Rawtherapee ?

I completely agree. I'm of the dual mind whether to handle this in the MacOS folder script or to handle in the code. The code would be better in which case some other issues would solve themselves.

Mainly, when I say 'RT standard' I mean that please share/commit the changes you make to the executable loader script and the bundle/packaging script (tools/osx/executable_loader.in and tools/osx/macosx_bundle.sh)

Absolutely. I was not sure how this collaboration worked and was hesitant to impose "my standards" on the RT community. Mainly I think, I was looking for some sort of green light from the owners like @Beep6581 before committing anything from my side.

I hope I am making sense. 🙂 Please let me know otherwise.

[Beep6581]

I support committing the required macOS changes once you agree on the best and standards-compliant way of handling them. I'm happy this is taking place!

[ion12]

@partha I couldn't figure out a way to separate the RT DATADIR from the BINDIR in the bundle/loader scripts either, but I think the changes necessary to the cmake files are not that extensive, and are an acceptable solution, assuming @Beep6581 agrees of course.

Here are the changes I made to the root CMakeLists.txt file:
CMakeLists.patch.zip
Note that I 'brute-force' changed settings, just to test and illustrate. A proper commit should of course wrap all the changes in if(APPLE) conditionals and leave the previous settings unchanged for non APPLE systems.

The re-placing of the LIBDIR and the rest of the required adjustments are handled in RawTherapee/tools/osx/macosx_bundle.sh and RawTherapee/tools/osx/executable_loader.in
Here are all the changes I recently made to accomodate for gtk3, Adwaita icons - and now to the bundle structure and user config/cache folder paths:
macosx_bundle.patch.zip
executable_loader.patch.zip

The current bundle/loader scripts, I think, are a hodge-podge of patch upon patch accumilated over time, with lots of mess and obsolete code. @partha I hope it's not too much of a burden to look at what I did because the end product of the 3 patches I attach here is a very clean bundle structure, with nothing but the binary and the loader in the 'MacOS' folder, all the dynamic libraries in their own folder, and everything else in the 'Resources' folder. (hopefully without any overlooked mistakes).

I chose 'lib' rather than 'Frameworks' for the LIBDIR folder name, because to the best of my understanding the unix style of bundeling dynamic libraries doesn't fully represent the Apple '.Framework' package concept, and putting them in a 'lib' folder reflects better and seems to be very common practice in unix-based .app bundles that doesn't actually violate Apple standards.
But if you feel that 'Frameworks' is a better choice, then by all means, it's just a matter of changing one line in both the bundle and loder scripts, renaming the folder to whatever you like.

@Partha1b @Beep6581 It would indeed be a great opportunity to completely clean up and re-write the bundle/loader scripts, I hope what I attach here could be made to good use.

A few final questions:

• Currently rtdata/CMakeLists.txt places the 'themes' dir at the root of the DATADIR while macos_bundles.sh creates another 'themes' at DATADIR/share. Why the discrepency?
• The RT code seems to consider fonts irrelevant for MacOS packaging, unless we use the x11 backend (which we don't). @Partha1b you are including fonts. Curious to understand whether or not this is required.
• As previously mentioned, the full set of Adwaita icons is ~50MB. I would really like to avoid packaging unused icons when it increases the final package's size so much, unless necessary.

thanks, hope this helps :)

Edit: I also noticed, with the current executable_loader, that other than GTK_IM_MODULE_FILE, GDK_PIXBUF_MODULE_FILE, XDG_DATA_DIRS all other GTK/Glib related path variables exports seem to be redundant?

[gaaned92]

@ion12 if it can help, for windows builds, following the advices from @sguyader and @TooWaBoo, I pack:

 adwaita/16x16:  /actions, /devices, /mimetypes, /places, /status
 adwaita/48x48: /devices
 adwaita/cursors: 5 windows cursors provided by toowaboo: sb_right_arrow, sb_left_arrow, sb_v_double_arrow, sb_h_double_arrow, plus

taking 1,45MB

[Beep6581]

@ion12 it would be much easier to see and discuss changes if instead of zip files you could fork and open a pull request.

It would be good for these changes to make it into 5.1.

[ion12]

@Beep6581 I've updated pull request #3706 to include the new suggestions for bundle structure & user folder paths... hope this is comfortable and useful for @Partha1b as well

@gaaned92 I have used your recommendation for icon selection. Not using cursors for now because they don't seem to be required on MacOS (?) Thanks!
I wanted @Partha1b 's input because he did choose to include the entire icon set and wondering if it's actually required in his opinion.

[Partha1b]

@ion12 & @Beep6581 Sorry, I've been very busy with other stuff. I'll look into what you proposed and get back to you.

[Partha1b]

@Beep6581 & @ion12 Can we create a fork so that we make changes to that fork and merge back when we're satisfied?

[Beep6581]

@Partha1b absolutely!

[ion12]

@Partha1b will you be making a new fork or do you want me to add you as a collaborator to the fork I have already made, maybe make a branch specifically for macOS packaging?
Not sure if you are just busy or if you are waiting for some response, let me know how I can help and keep @Beep6581 updated as well? :)

[Partha1b]

@ion12 & @Beep6581 Apologies since I've very busy with other stuff and as usual, real life intrudes.

After I asked if I could fork, I cloned the latest master and began looking into how to make it compatible with how Mac apps are usually set up. I am able to have the settings set to ~/Library/Application Support/Rawtherapee within options.cc and so we're good there.

@ion12 I am comfortable with putting all supporting dylibs in the Framework folders as I have seen other native apps (like Photoshop). I am also OK with putting ratherapee-bin within the MacOS folder and remove the bin folder.

I've been testing some of these on my HD so as not to clutter up github. But the primary reason is, I didn't know how to create a separate branch and so was trying to get things working on my machine first.

The above are good, but I have one problem with absolute paths being an absolute requirement and was seeing how to work around that. Seems to affect the camconst.json file so far. Any way we can make that file reside in the data directory?

Thanks,
Partha

[ion12]

@Partha1b could you point out where in the code is the issue with the camconst.json file path?

[Partha1b]

@ion12 In file RawTherapee/rtengine/camconst.cc Line 699 (in my case):

void CameraConstantsStore::init(Glib::ustring baseDir, Glib::ustring userSettingsDir)
{
    parse_camera_constants_file(Glib::build_filename(baseDir, "camconst.json"));

    Glib::ustring userFile(Glib::build_filename(userSettingsDir, "camconst.json"));

    if (Glib::file_test(userFile, Glib::FILE_TEST_EXISTS)) {
        parse_camera_constants_file(userFile);
    }
}

And the location is fixed due to RawTherapee/rtengine/CMakeLists.txt Line 45:

install (FILES ${CAMCONSTSFILE} DESTINATION "${DATADIR}" PERMISSIONS OWNER_WRITE OWNER_READ GROUP_READ WORLD_READ)

And the datadir is required to have an absolute path unless you are building a bundle. Building a bundle means all files are dumped into the install-prefix folder.

[Beep6581]

@Partha1b

After I asked if I could fork, I cloned the latest master

master is long dead. Use the dev branch.

[Partha1b]

@Beep6581 Sorry, I meant dev.

[Partha1b]

I don't see your comment on github, @ion12?? :(

Anyway, yes, you misunderstood.

I am building by removing the bundle and absolute path requirements in the
CMakeList.txt file.

[ion12]

@Partha1b sorry...

RawTherapee/CMakeLists.txt
lines 115-118

# set install directories
if (WIN32 OR APPLE)
    set (BUILD_BUNDLE ON FORCE)
endif(WIN32 OR APPLE)

why remove this?

and there should be a camconst.json in the DATADIR, and a second camconst.json can be created by the user and placed in the user config dir next to the user option file.
Does this not work?

[Partha1b]

@ion12 If I remove APPLE from above, then we can get bin/rawtherapee and share/ with the data. Now, I can do the following: bindir=./MacOS and datadir=./Resources/ except for the requirement that without bundle these folders have to have absolute paths.

[ion12]

@Partha1b did you look at the changes I made to the cmake files and original build scripts?
https://github.com/Beep6581/RawTherapee/pull/3706/files
This works without removing the bundle option.
If there is an advantage to your method than by all means, I admit I don't know what is better.. maybe @Beep6581 or someone else can advise :)

(I just noticed that I accidently left the CreateDmg function call commented out, will fix that)

[Partha1b]

@ion12 Yes, your method works fine. I should have looked at it more closely. 🙁

Only 2 things left to fix.

1. Create the settings locations within the program.
   To do this, the following should work. I am sure @Beep6581 could probably do better with the code:

+++ options.cc  2017-03-08 19:30:18.000000000 -0500
@@ -2188,6 +2188,8 @@ bool Options::load ()

     const gchar* path;
     Glib::ustring dPath;
+   std::string rtdir1, homedir, cachedir;
+   homedir = g_getenv("HOME");

     path = g_getenv ("RT_SETTINGS");

@@ -2206,7 +2208,13 @@ bool Options::load ()
             WideCharToMultiByte (CP_UTF8, 0, pathW, -1, pathA, MAX_PATH, 0, 0);
             rtdir = Glib::build_filename (Glib::ustring (pathA), Glib::ustring (CACHEFOLDERNAME));
         }
-
+#elif __APPLE__
+        rtdir1 = homedir +  "/Library/Application Support/RawTherapee";
+       if (options.rtSettings.verbose) 
+       {
+           printf ("Settings directory (rtdir1) = %s\n", rtdir1.c_str());
+       }
+       g_mkdir_with_parents (rtdir1.c_str(), 0755 );
 #else
         rtdir = Glib::build_filename (Glib::ustring (g_get_user_config_dir ()), Glib::ustring (CACHEFOLDERNAME));
 #endif
@@ -2236,6 +2244,10 @@ bool Options::load ()
     else if (options.multiUser) {
 #ifdef WIN32
         cacheBaseDir = Glib::build_filename (rtdir, "cache");
+#elif __APPLE__
+        cachedir = homedir +  "/Library/Application Support/RawTherapee/cache";
+        printf ("Settings cachedir = %s\n", cachedir.c_str());
+       g_mkdir_with_parents (cachedir.c_str(), 0755 );
 #else
         cacheBaseDir = Glib::build_filename (Glib::ustring (g_get_user_cache_dir()), Glib::ustring (CACHEFOLDERNAME));
 #endif

1. Would you be opposed to putting all the support libs in the Framework folder? If not, we can make the appropriate changes in your script.

[ion12]

@Partha1b

1. I just re-built and double checked and as far as I can tell, the detection of the config/cache folder under "~/Library/Application Support/RawTherapee" is working fine with the changes I made without the need to make further changes in the options.cc file.
   Is it possible that the options.cc changes were necessary because you disabled building as bundle?
   Would be great if you can confirm.

1. I suggested using "lib" instead of "Frameworks" because RT isn't using ".Frameworks" macOS packages.
   I noticed quite a few unix-based applications choose "lib" for dynamic shared libraries and it seems to be acceptable to use customed-named folders when appropriate under mac standards.
   But if you feel "Frameworks" is a better choice, by all means. I've seen both options being used.
   What I do think should be maintained is that the libraries should be under either "Contents/lib" or "Contents/Frameworks" but not under "Contents/Frameworks/lib". Hope this makes sense.

[Partha1b]

@ion12

I just re-built and double checked and as far as I can tell, the detection of the config/cache folder under "~/Library/Application Support/RawTherapee" is working fine with the changes I made without the need to make further changes in the options.cc file.
Is it possible that the options.cc changes were necessary because you disabled building as bundle?
Would be great if you can confirm.

Yes this works of course. That's my old solution. I was intending to have a solution within the code where the folders would be created if they didn't exist.

I am OK with your 2nd solution as well. We can provide all the support libs within the Frameworks folder and thereby future proof it in case we decide to switch to using OSX frameworks.

I'd say then we're done and you can merge this back to the main dev branch?

[Beep6581]

If there is an advantage to your method than by all means, I admit I don't know what is better.. maybe @Beep6581 or someone else can advise :)

I'm willing to help how I can, but I can't advise on macOS things as I'm unfamiliar with its structure, requirements and best practices.

I'm also preparing to leave to Uganda for a week on Sunday, so I won't be around until the ~19th.

[ion12]

@Partha1b I'll rename "lib" to "Frameworks" then, sure :) I won't have time today but will do it by tomorrow, and then @Beep6581 you can merge if all lookes good to you.
but -
@Partha1b @Beep6581 If we are merging these fixes to the original scripts do you think you could provide some insights in terms of removing all the obsolete code in them?
The original packaging scripts work very well as a basis, but clearly over time patch upon patch was added and there are surely still some bits of left-over code in there that are now redundant.
I never really felt comfortable offering a complete clean-up by myself, but perhaps with your combined experience it's a good opportunity to do so for the next release? ( @Beep6581 I think some general unix know-how is just as valuable to clean up the scripts, and we can point out if there are specific macOS issues to consider)

edit - I'm happy to leave the final changes till after @Beep6581 comes back, I also have a tough work week ahead ;)

[ion12]

I renamed 'lib' folder to 'Frameworks' folder as per @Partha1b's recommendation in the macOS build/loader scripts and updated the pull request.
@Beep6581 :

1. For the CMakeLists.txt file - I will add an if (APPLE) conditional for the BINDIR and DATADIR locations.
2. There are 6 additional directories being set (LICENCEDIR, ICONSDIR etc.) - would you consider it acceptable to replace "." with "${DATADIR}" which will work for both UNIX and APPLE instead of adding 6 if (APPLE) conditionals?
3. What is the DOCDIR being used for? It doesn't seem to be used for macOS bundling at all.
4. The current scripts create on macOS 2 different "themes" folders, one inside DATADIR and one inside DATADIR/share. Should we consolidate into one folder (and if so which one?) or is that intentional?

[Beep6581]

I opened a local branch to clean up the CMake files some weeks ago, specifically I don't like having stuff for Windows, macOS and Linux in one file AND having a separate win.cmake file - it would be cleaner to combine the two. The branch is out of sync now and does not merge - I'll get back to that in some weeks.

@ion12

1. Sounds acceptable.
2. Nothing, apparently. I would make the man page use it - currently the man page uses ${PROJECT_SOURCE_DIR}/doc/manpage/rawtherapee.1
3. DATADIR/share would be ${CMAKE_INSTALL_PREFIX}/share/rawtherapee/share/ and that's wrong.

[ion12]

@Beep6581 @Partha1b Sorry for the long wait, I will try to get around to closing these last details in the next couple of days, and of course @Partha1b if you still have any input/proposed changes... :)

[ion12]

@Beep6581 @Partha1b
Before I commit the changes to the pull request, a few last questions:

1. 'Contents/Resources/share/man/man1' subfolder will be replaced with ‘Contents/Resources/doc’ on mac.
   @Beep6581 - should './share/man/man1' also be replaced with ‘./doc’ on linux, or did you suggest the change just for mac?

2. The old ‘macosx_build.sh’ scripts placed the mac gtk themes in ‘./share/themes’ while the RT themes are created in ‘./themes’
   @Beep6581 so to confirm ALL themes should go into './themes' ? ( ‘Contents/Resources/themes’ on mac? )

3. FONTCONFIG_PATH and /etc/fonts are currently ONLY setup for X11.
   Is it safe/preferable to remove the code for this from ‘macosx_build.sh’ and ‘executable_loader.in’ ?
   I commented it out and can see no issues (which makes sense since we are no longer using X11 backend?) but I don’t want to remove completely without confirmation…

4. In ‘executable_loader.in’:
   Is the ‘libcups’ hack still necessary? I commented it out and the build seems to work fine without it but I don’t really understand why it was required in the first place.

5. In ‘executable_loader.in’:
   What is the purpose of
   case $1 in -psn_*) shift;; esac
ln -sf "${app}" /tmp
   and is it still required?

6. In ‘executable_loader.in’:
   Since the data/resource folders are now set up correctly in the CMakeLists file, I see no further use for setting up GTK_EXE, GTK_DATA_PREFIX, GTK_IM_MODULE_FILE, GDK_PIXBUF_MODULE_FILE, XDG_DATA_DIRS manually, build seems to run fine without it.
   The only 3 settings I think are required are DYLD_LIBRARY_PATH, RT_SETTINGS, RT_CACHE
   @Partha1b can you confirm?

thanks

[Beep6581]

@ion12

1. Both "man1" and "doc" exist in Linux. I don't know the best practices for which one to use yet.
2. I can only confirm that DATADIR/share would be wrong.
3. Since we no longer use X11 on macOS then it makes sense to disable those lines. If the script could still work with X11 then I would comment the lines out and comment why they're commented out. If the script does not work with X11 (i.e. would require radical changes) then you could delete those lines entirely.
4. I don't know about why that's there. If it works without that, should be safe to remove while we're still in testing (i.e. not releasing 5.1 yet).
5. The way that script is written makes my eyelids twitch. I will re-write it. The code you quoted consists of two discrete lines. The first checks whether the first positional parameter is -psn_* which is explained here http://stackoverflow.com/questions/10242115/os-x-strange-psn-command-line-parameter-when-launched-from-finder If that parameter exists, shift moves on to the next one, ignoring the first one. The second creates a symlink in /tmp to the folder which contains the script.
   Delete these lines if RT works without them. Test whether it works when run from a terminal and from "Finder".

[Partha1b]

@ion12 & @Beep6581

My comments below in the order of the questions above:

1. Man is manual and doc can be general docs. I'd put man as doc/man
2. I'd still keep them all in shared resources. In other words, ./Resources/share/themes
3. I am pretty sure you still need FONTCONFIG_PATH for freetype and fontconfig dylibs.
4. Don't know about libcups. I don't use it and seems to be fine for me.
5. I have the following in my scripts:

# Strip out the argument added by the OS.
echo "Strip out the argument added by the OS..."
if /bin/expr "x$1" : '^x-psn_' > /dev/null; then
    shift 1
fi

1. At a minimum, I'd definitely retain the following:

export GTK_PATH="/tmp/RT/Contents/Frameworks/lib/gtk-3.0/3.0.0"
export XDG_DATA_HOME="/tmp/RT/Contents/Frameworks/share"
export GSETTINGS_SCHEMA_DIR="/tmp/RT/Contents/Frameworks/share/schemas"

or wherever you're putting the schemas and gtk modules.

[Beep6581]

I'd put man as doc/man

Do you mean you'd put repo/doc/manpage/rawtherapee.1 into /usr/share/doc/man/rawtherapee.1? There is no /usr/share/doc/man folder in Sabayon.

[Partha1b]

@Beep6581
Sorry, I should have been clearer. I mean that we have in the CMakeList.txt the following:

if (UNIX)
    install (FILES "${PROJECT_SOURCE_DIR}/doc/manpage/rawtherapee.1" DESTINATION "${CMAKE_INSTALL_PREFIX}/share/man/man1")
    install (FILES rawtherapee.appdata.xml DESTINATION "${APPDATADIR}")
endif (UNIX)

I am thinking that for the Mac, we could the following:

if (APPLE)
    install (FILES "${PROJECT_SOURCE_DIR}/doc/manpage/rawtherapee.1" DESTINATION "${CMAKE_INSTALL_PREFIX}/Resources/share/doc/man")
    install (FILES rawtherapee.appdata.xml DESTINATION "${APPDATADIR}")
endif (APPLE)

[Beep6581]

If foo/Resources/share/doc/man is standard in macOS land then sure.

[Beep6581]

Please test this revised tools/osx/executable_loader.in:
https://paste.pound-python.org/show/Ox7SxnulgU5w2AZl7MLo/

The changes I made do not take into account the last few comments above, so delete the lines which are no longer required.
The last line uses exec. I see no need for it, but left it in place. You could remove the "exec" part and test if it works.

[Partha1b]

@Beep6581

If foo/Resources/share/doc/man is standard in macOS land then sure.

Mac OS standard is the same as BSD: manpages are located in /usr/share/man or /usr/local/share/man. But we don't want to put RT manpage there, do we?

[Beep6581]

But we don't want to put RT manpage there, do we?

Not if it's a bundle build. But why foo/Resources/share/doc/man and not foo/Resources/share/man? Anyway you have this under control, so just pick what seems most correct and we'll go with that.

[Partha1b]

@Beep6581
Fine with me to go with foo/Resources/share/man. I was considering man as a reference doc and hence my suggestion.

[ion12]

@Beep6581 @Partha1b

@Beep6581 Can you make a clear call about the manpage and themes dir locations, because you and @Partha1b have made different suggestions, and I have no opinion on this one way or the other.

1. 'DATADIR/share/doc/man' or 'DATADIR/share/man' ? (WIN32 manpage install is handled separately in /rtdata/CMakeLists.txt for some reason, I won't touch that code but for your attention...)
2. 'DATADIR/themes'' or 'DATADIR/share/themes' ?
3. I tried removing the X11 conditional with the current scripts and installing fonts into 'etc/fonts' and setting FONTCONFIG_PATH in the loader script. I cannot detect any difference with or without it.
4. I will comment this out.
5. Ran from Finder, from terminal, ran loader script directly in terminal, called RT from a photo browser's (XnView) 'open with' command - No difference detected with or without theses two lines. @Partha1b can you confirm the psn shift and creating a symlink of the .app bundle in /tmp are really necessary?
6. @Partha1b I will include the lines you suggest are necessary and comment out the rest. But even those lines don't seem to make any difference once the DATADIR is correctly defined in CMakeLists (although I might be missing something...)

@Beep6581 the last 'exec' line is what makes the loader actually load the binary itself, it's necessary.

@Partha1b I have discovered one bug that exists with the current build scripts /macport toolchain that does NOT exist in your build - If try to open in RT a jpg created from a photoshop file containing text layers, RT crashes reporting:
Assertion failed: (!scaled_font->cache_frozen), function _cairo_scaled_glyph_page_destroy, file cairo-scaled-font.c, line 459.
Oddly enough, this crash only happens if this kind of file is the first file that you try to load in the editor after starting RT. If you first load into the editor a standard photo file and then try to load the jpg with the text, RT handles it fine.
I noticed you build includes quite a few cairo/pango libraries that do not get bundled with my macports build

[Beep6581]

1. What value will datadir have in macOS if not building a bundle?
2. Pending on answer to question 1.
3. Good. If not needed then let's not have it (until someone complains).
   4.
4. No idea why the symlink is there in the first place.
   6.
5. exec invokes a sub-process. If macOS works like Linux in this regard then it doesn't seem to be needed. Note that I'm talking about the exec command alone, not the whole line.
6. Does the crash happen if you include the fontconfig dylib stuff?

[ion12]

CMakeLists includes

# set install directories
if (WIN32 OR APPLE)
    set (BUILD_BUNDLE ON FORCE)
endif(WIN32 OR APPLE)

Is a non-bundle build even relevant for mac/win?
In any case, I am only asking about adjusting the settings under if (BUILD_BUNDLE) conditionals.

1. Sorry, misunderstood. Yes, removing the exec command doesn't seem to impact negitavely, although I don't know if there is a macOS-specific reason for using it that is not relevant to linux.
2. Yes, although maybe I'm including it wrong (using the old code, just commenting out the X11 conditional)

[Beep6581]

I had a discussion with @agriggio in IRC about exec and we decided that the moon landing was definitely staged. We also agreed that using it is fine (it does not invoke a sub-process - I got that info from man exec which is actually wrong because that man page is for TCL and exec is a Bash built-in making man bash the correct source of info). Then again I can't verify this in macOS as I don't use it. But the landing was definitely fake.

[Partha1b]

@ion12

@Partha1b I have discovered one bug that exists with the current build scripts /macport toolchain that does NOT exist in your build - If try to open in RT a jpg created from a photoshop file containing text layers, RT crashes reporting:
Assertion failed: (!scaled_font->cache_frozen), function _cairo_scaled_glyph_page_destroy, file cairo-scaled-font.c, line 459.

Yes, we had a long discussion on this on the discussion board. I did make some changes to try and fix it. I think I fixed it, but not totally sure. Anyway, check that thread.

Also, all the libs I include are needed. I package the minimum needed mostly. 🙂

@Beep6581

Use exec or not.

I think it's better to keep it. I use it in my scripts.

@Beep6581

What value will datadir have in macOS if not building a bundle?

datadir will be the share folder.

[ion12]

@Beep6581 @Partha1b I updated the pull request to the best of my understanding according to the last few comments. You still might want to make some changes to naming/structure.
I didn't comment out the psn / symlink code in the executable_loader nor all the GTK related path exports even though I think it's safe to do so, as @Partha1b did not confirm/suggest they should be safely left out. The code that I did comment out should be safe to delete alltogether. I leave the final decisions to you.

Sorry @Partha1b I tried going over the forum threads but I still can't figure out what you did to avoid the jpg related crash, if it's a cairo issue or incorrectly configured fonts/fontconfig installation.
Hopefully you could incorporate your fixes into the new revised build scripts?

@Beep6581 - Now that you've come to the conclusion the the moon landing was faked, it's time to deal with the really big question... is the _Flat Earth Society_ actually on to something?
:)

[Beep6581]

    if (APPLE)
        set (BINDIR "${CMAKE_INSTALL_PREFIX}/MacOS")

Is it supposed to be MacOS or macOS? If macOS then also change in tools/osx/macosx_bundle.sh

I made revisions, please add these to the PR:
https://github.com/Beep6581/RawTherapee/commit/26c29a563b27e523ee333117c8b705a9f3019e18

[Partha1b]

@Beep6581 It should be MacOS.

[Partha1b]

@ion12 said:

as @Partha1b did not confirm/suggest they should be safely left out.

I definitely commented on this above here: https://github.com/Beep6581/RawTherapee/issues/3678#issuecomment-288370507

[Partha1b]

@ion12 Forget to address the cairo issue. I looked through the code and didn't think that that particular line in cairo-scaled-font.c added anything to the mix and commented it out.

[ion12]

@Partha1b I saw the comment, that's why I left in the code per your recommendation :)
What I meant is that subsequently I detected no negative effect, by trial and error, in removing it - and you did not confirm/suggest that it would be ok to do so.
Same goes for the psn shift code and the symlink to /tmp. I can not detect any issue with removing them but won't suggest doing so as I don't fully understand their purpose.

@Partha1b specifically in regards to the /tmp symlink: I see in your executable loader that you actually use the symlink to define some of the GTK paths. Why is that prefereable to linking to the bundle folders? In the old loader script a symlink is created but isn't used for anything, so would be nice to clean this up one way or another.

@Partha1b thanks for the cairo tip, will try it

@Beep6581 @Partha1b
Still the 'themes' dir decision - '/rtdata/CMakeLists.txt' creates 'DATADIR/themes' while 'macosx_bundle.sh' creates 'DATADIR/share/themes' for the GTK-mac themes.
Whatever the correct location, let's not create 2 separate 'themes' dirs?

@Beep6581 I merged your commit into the pull request, I'm still learning the ins-and-outs of github, I hope I did it correctly

[ion12]

@Beep6581

    if [[ ${CMAKE_BUILD_TYPE,,} != release ]]; then
        dmg_name="${dmg_name}_${CMAKE_BUILD_TYPE,,}"

results in
/tools/osx/macosx_bundle.sh: line 209: ${CMAKE_BUILD_TYPE,,}: bad substitution

[Beep6581]

@ion12 can you come to IRC? http://rawpedia.rawtherapee.com/IRC

[ion12]

@Partha1b @Beep6581 patched cairo removing the !scaled_font assertions on lines 459-460 of 'cairo-scaled-font.c' according to @Partha1b 's recommendation and the crash is indeed resolved. Great!
But I guess this means we now need to provide a patch for cairo like we do for libiconv to provide a repeatable macports based installation?

[ion12]

note that I do get the odd crash AFTER quitting RT:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       EXC_I386_GPFLT
Exception Note:        EXC_CORPSE_NOTIFY

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libomp.dylib                    0x0000000105d2bc6d __kmp_dephash_free_entries + 21
1   libomp.dylib                    0x0000000105d2bdcb __kmp_dephash_free + 18
2   libomp.dylib                    0x0000000105d06580 __kmp_free_implicit_task + 30
3   libomp.dylib                    0x0000000105cfe699 __kmp_reap_thread(kmp_info*, int) + 123
4   libomp.dylib                    0x0000000105cfd519 __kmp_internal_end_library + 493
5   dyld                            0x00007fff65fd73bc ImageLoaderMachO::doTermination(ImageLoader::LinkContext const&) + 212
6   dyld                            0x00007fff65fc618b dyld::runAllStaticTerminators(void*) + 67
7   libsystem_c.dylib               0x00007fff9144f463 __cxa_finalize_ranges + 345
8   libsystem_c.dylib               0x00007fff9144f767 exit + 55
9   libdyld.dylib                   0x00007fff967b15b4 start + 8

I don't know if it's related to the cairo patch (or to the packaging issues at all, maybe something I just never noticed before) and it doesn't always happen so I'm not sure what is the exact trigger for this.

[Beep6581]

@ion12 @Partha1b new branch osxbuild

Reminder that we changed manpage location for UNIX in '/CMakeLists.txt' but not for Windows (set separately in 'rtdata/CMakeLists.txt')

Fixed.

Reminder that we are using @Partha1b's fix to patch cairo (link). To help other users build correctly we should add this patch to the repo as you did for libiconv, or at least defer this for a seperate issue.

I see no patch, and I have no file called cairo-scaled-font.c on my system. If this is still required, please provide the patch and describe which package and version it applies to.

[Partha1b]

@Beep6581 The file cairo-scaled-font.c is part of the cairo package. When I checked that, I commented out the assert statement which was throwing up unnecessary segfaults in some cases.

[heckflosse]

@ion12 @Partha1b There are some reports about latest MacOS builds being slow. Now I read about asserts which usually are only enabled for Debug builds. Can it be that latest MacOS builds have been debug builds?
If not, we should try to find the reason for slowness of the builds.

Ingo

[Partha1b]

@heckflosse & @Beep6581
I have not seen the reports. Probably because I was not copied on them. 😦

My builds are release builds, not debug. However, you have to note that we are required to build the MacOS version for a generic processor while I am not aware of any non-intel Mac hardware running 10.9 or higher. It should speed up if we build at least for 64-bit intel processors.

cc: @ion12

[heckflosse]

@Partha1b

It should speed up if we build at least for 64-bit intel processors.

I would appreciate that!!!

[heckflosse]

@Partha1b If someone, including me possibly, required you to make generic builds, there may be a misunderstanding.
Of course we don't want builds which are made for a certain CPU because these builds will crash when run on a different cpu. But a generic x86 and a generic x84/64 build are different worlds. A generic x86/64 build will enable SSE2 by default which performancewise is way better than a generic x86/32 build.
A generic x86/64 build should work fast on every x86/64 cpu when you run it on a x86/64 MacOS

[Partha1b]

@heckflosse I should continue to git pull from the dev branch, correct?

[heckflosse]

@Partha1b Yes, of course. Though the branch has no influence on the target of your builds.

[Partha1b]

@heckflosse Merely making sure that I don't pull "obsolete" branches. 😄

[heckflosse]

:)

[heckflosse]

@Partha1b If you want to verify that you are making builds with SSE2 support insert

#error nonssebuild

after the

#else

in line 246 of rtengine/amaze_demosaic_RT.cc

If the build fails it's a non SSE2 build

Ingo

[Partha1b]

@heckflosse No build failure. So I am building SSE2 build.

[Beep6581]

@Partha1b

The file cairo-scaled-font.c is part of the cairo package. When I checked that, I commented out the assert statement which was throwing up unnecessary segfaults in some cases.

I don't have that file nor a patch for it. If you would like me to include it in tools/osx, which is probably a good idea if you're using such a patch, then please send it to me and describe which version of cairo it's for.

Branch osxbuild has not been merged to dev yet because it causes an issue on Linux. Looking into it later today.

[Beep6581]

Reported via email, needs to be fixed:

Before:
/usr/share/icons/hicolor/XXxXX/apps/rawtherapee.png
After:
/usr/share/rawtherapee/share/icons/hicolor/XXxXX/apps/rawtherapee.png

Before:
/usr/share/appdata/rawtherapee.appdata.xml
After:
/usr/share/rawtherapee/share/appdata/rawtherapee.appdata.xml

Before:
/usr/share/applications/rawtherapee.desktop
After:
/usr/share/rawtherapee/share/applications/rawtherapee.desktop

Before:
/usr/share/man/man1/rawtherapee.1
After:
/usr/share/doc/packages/rawtherapee/man/rawtherapee.1

[Beep6581]

@ion12 @Partha1b what was the reason for replacing ${CMAKE_INSTALL_PREFIX} with ${DATADIR} for Linux? e.g.

 if (NOT DEFINED ICONSDIR)
     if (UNIX)
-        set (ICONSDIR "${CMAKE_INSTALL_PREFIX}/share/icons")
+        set (ICONSDIR "${DATADIR}/share/icons")
     endif (UNIX)
 endif (NOT DEFINED ICONSDIR)

[Partha1b]

@Beep6581 The iconsdir for unix was probably an error. @ion12 do you know the reason for the change?
@Beep6581 I didn't understand the previous comment above about stuff that needs to be fixed. Is that for the Mac or general?

[Beep6581]

@Partha1b I believe it was for a Linux non-bundle build. I'm waiting for confirmation of the CMake parameters used and whether the build dir was clean.

[Beep6581]

My question is specifically about DESKTOPDIR, ICONSDIR and APPDATADIR which concern for Linux only - was the change a fix for something?

-        set (APPDATADIR "${CMAKE_INSTALL_PREFIX}/share/appdata")
+        set (APPDATADIR "${DATADIR}/share/appdata")

[Beep6581]

Commit https://github.com/Beep6581/RawTherapee/commit/5ff36958fa752e173e09cb60dc4e4ed9b96b2e67 reverts handling of some folders to the way it was previously for Linux, but changes remain for Apple (and Windows?). @ion12 @Partha1b and others, please test using
-DBUILD_BUNDLE="OFF" and ON.

[Partha1b]

@Beep6581

My question is specifically about DESKTOPDIR, ICONSDIR and APPDATADIR which concern for Linux only - was the change a fix for something?

Do you mean these variables are not to be set for Apple/Windows or do you mean that you are concerned about the changes for Linux only?

Note that I do set these variables in my builds.

[Partha1b]

@Beep6581

@ion12 @Partha1b and others, please test using -DBUILD_BUNDLE="OFF" and ON.

The variable BUILD_BUNDLE has no affect on Mac/Windows. The bundle is forced on these platforms. Of course, one can modify the cmakelist.

[Beep6581]

I pushed a functional change, please test. I hope it works. If it does, I will clean up the style of the CMake files before merging.

[Beep6581]

I pushed a style cleanup of /CMakeLists.txt in commit c754a69a. It makes no functional changes. The sub-folder CMake files are next.

[Beep6581]

A rebase seems to have messed things up, introduced commits which shouldn't be in the osxbuild branch and #3807 PR. I deleted the osxbuild branch and PR.

Use the new osxbuild2 branch.

[Beep6581]

Incorrect man page location in Linux should be fixed.

[Beep6581]

Linux:
cmake -DBUILD_BUNDLE="OFF" -DCMAKE_INSTALL_PREFIX="/home/beep6581/programs/rawtherapee"

[Beep6581]

osxbuild2 merged into dev.

@ion12 @Partha1b please open a new issue with the cairo patch for inclusion in /tools/osx/ including the cairo version the patch is for, and anything else that's needed.

A new macOS dev build would be great.

[Partha1b]

@Beep6581 make macosx_bundle fails for me. The error I get is:
-- Copying dependencies from /Users/partha/local10.9: ditto: No destination Usage: ditto [ <options> ] src [ ... src ] dst
for all the libs. I am not sure why the ditto command fails.

I am happy to create the app the way I've been doing so far. Are you OK with that?

[Beep6581]

@Partha1b new builds are always welcome, but in this case that would defeat the purpose of getting this working and tested.

Could you add set -x as the first line of tools/osx/macosx_bundle.sh and run it again, then paste the full log either here or using http://paste2.org/ ? In fact you can open a new issue for that.

[ion12]

@Beep6581 @Partha1b @heckflosse Just got back from travel this evening, and will need to catch up if there is still some testing/patching to be done. I can do so over the weekend if it's still helpful.

Quickly, regarding the issue with DESKTOPDIR, ICONSDIR and APPDATADIR : I think it is me failing to notice that these are not setup with a conditional for bundle/non-bundle builds and so the change I made was wrong for non-bundle linux build.
Are these files even necessary for a macOS bundle? If they are, then maybe they should be setup similarly to DOCDIR CREDITSDIR and LICENSEDIR, I'm not sure that they should be installed based on ${CMAKE_INSTALL_PREFIX} for a macOS bundle in the same way as they are installed for linux...

[Partha1b]

@ion12 I simply add them to my CMakeList.txt and put them in the Resource folder. For example:

if (DEFINED BUILD_BUNDLE)
    set (DESKTOPDIR "${CMAKE_INSTALL_PREFIX}/Resources/share/applications")
    set (ICONSDIR "${CMAKE_INSTALL_PREFIX}/Resources/share/icons")
    set (APPDATADIR "${CMAKE_INSTALL_PREFIX}/Resources/share/appdata")
endif (DEFINED BUILD_BUNDLE)