Opened 6 years ago

Closed 6 years ago

#385 closed enhancement (fixed)

Add OS X framework build option to cmake

Reported by: kyngchaos Owned by: Mateusz Łoskot
Priority: minor Milestone: GEOS Future
Component: Build/Install Version: svn-trunk
Severity: Unassigned Keywords: cmake
Cc:

Description

patch and new file attached. Two new cmake settings:

GEOS_ENABLE_MACOSX_FRAMEWORK -- enables the framework build. I wanted to leave the option of building a standard pair of libraries on OS X. Default ON.

GEOS_ENABLE_MACOSX_FRAMEWORK_UNIXCOMPAT -- adds my "unix compatibility" feature, that makes a standard unix bin|include|lib structure with symlinks to the corresponding framework parts available to framework-challenged unix ports. Default OFF.

The CMAKE_INSTALL_PREFIX must be set in configuration, since the default /usr/local is not appropriate for frameworks. Since CMAKE_INSTALL_PREFIX defaults from cmake itself, I wasn't sure if/how I could default it to /Library/Frameworks? if the user doesn't set it.

I used builtin cmake target features for creating a framework when possible. A couple things needed some (sometimes messy) customization (don't worry, it doesn't affect other systems).

  • For a framework, I changed the name of the library to all caps, "GEOS". Case shouldn't matter on most Mac systems, but a case-sensitive file system is an option on OS X (not recommended), so I post-process the headers to match.
  • A framework is a single binary library, so I build the C API into the main C++ library. It's possible to do special linking to join multiple libraries into one, but I've had problems with that in the past.

Attachments (7)

mac_framework.patch (8.8 KB) - added by kyngchaos 6 years ago.
Info.plist.in (873 bytes) - added by kyngchaos 6 years ago.
info.plist template, goes in src/
tests.patch (2.8 KB) - added by kyngchaos 6 years ago.
supplementary patch for tests
CMakeLists.patch (717 bytes) - added by kyngchaos 6 years ago.
patch to add missing library versions to framework
src_CMakeLists.txt.patch (15.8 KB) - added by kyngchaos 6 years ago.
install_name_dir fix
capi_CMakeLists.txt.patch (461 bytes) - added by kyngchaos 6 years ago.
install_name_dir fix
bug#385.patch (1.1 KB) - added by kyngchaos 6 years ago.

Download all attachments as: .zip

Change History (29)

Changed 6 years ago by kyngchaos

Attachment: mac_framework.patch added

Changed 6 years ago by kyngchaos

Attachment: Info.plist.in added

info.plist template, goes in src/

comment:1 Changed 6 years ago by strk

Owner: changed from pramsey to Mateusz Łoskot

comment:2 Changed 6 years ago by Mateusz Łoskot

Status: newassigned

comment:3 Changed 6 years ago by Mateusz Łoskot

Keywords: cmake added
Resolution: fixed
Status: assignedclosed

William, thanks for the patch. Applied to the trunk r3375 . I have no way to test it on OSX, so I'd appreciated your help.

Sandro, all unit tests pass for me on Linux:

mloskot@dog:~/dev/geos/_svn/build$ uname -a
Linux dog 2.6.35-28-generic #50-Ubuntu SMP Fri Mar 18 18:42:20 UTC 2011 x86_64 GNU/Linux

mloskot@dog:~/dev/geos/_svn/build$ make test
Running tests...
Test project /home/mloskot/dev/geos/_svn/build
    Start 1: geos_unit
1/4 Test #1: geos_unit ........................   Passed    0.15 sec
    Start 2: xmltester
2/4 Test #2: xmltester ........................   Passed   11.17 sec
    Start 3: bug234
3/4 Test #3: bug234 ...........................   Passed    0.00 sec
    Start 4: TestSweepLineSpeed
4/4 Test #4: TestSweepLineSpeed ...............   Passed    2.66 sec

100% tests passed, 0 tests failed out of 4

Total Test time (real) =  13.98 sec

comment:4 Changed 6 years ago by kyngchaos

You forgot the info.plist.in. I added that as an attachment separate from the patch.

I put it at the top level of the source (src/info.plist.in) because it's a framework property of the main GEOS target, but maybe there's a more appropriate place? Maybe a macosx subfolder?

comment:5 Changed 6 years ago by strk

Resolution: fixed
Status: closedreopened

comment:6 Changed 6 years ago by strk

Resolution: fixed
Status: reopenedclosed

done in r3376.

Mat: note that TestSweepLineSpeed? isn't meant to be run as a regression test, as it doesn't compare actual behavior with any expected one. Also bug234 is not meant to be automatically tested. At least isn't by automake, not sure what the origin was, but should be tested either by geos_unit or xmltester..

comment:7 Changed 6 years ago by kyngchaos

That's got it. Thanks! Build tests OK.

I rarely run the unit tests and didn't consider that in the patch. I'll work on a supplement patch, but you don't need to let it hold up release.

Changed 6 years ago by kyngchaos

Attachment: tests.patch added

supplementary patch for tests

comment:8 Changed 6 years ago by kyngchaos

Resolution: fixed
Status: closedreopened

Patch added to support the unit tests. Tests run, but 1 failure (but that's not for this bug).

Changed 6 years ago by kyngchaos

Attachment: CMakeLists.patch added

patch to add missing library versions to framework

comment:9 Changed 6 years ago by kyngchaos

I can't believe I missed the library versions in the framework properties. Got all the framework version stuff. Patch above (for src/cmakelists.txt).

Note: it looks like the cmake library versions in general need adjustment. In the libtool build (at least on OS X), there was a calculation done so that the link version was 1+ the major version. On OS X this is the current and compatibility versions stored inside the library file. ie for GEOS 3.3 that would be 4.3.0. If someone builds the cmake GEOS 3.3 now, it will be 3.3.0, which is then seen as an earlier version than the previous 4.2 (3.2.x) to the linker, and programs will fail to load GEOS without a rebuild.

Maybe it's just a Mac thing, someone should verify this on *nix/windows. At least, a Mac non-framework build should set VERSION and SOVERSION as I do for the framework in the patch.

Changed 6 years ago by kyngchaos

Attachment: src_CMakeLists.txt.patch added

install_name_dir fix

Changed 6 years ago by kyngchaos

Attachment: capi_CMakeLists.txt.patch added

install_name_dir fix

comment:10 Changed 6 years ago by kyngchaos

OK, old news, more patches. Fix for linking - prefix the install_name in the library. This actually needs to be done for both the framework and the unix builds on OS X, so the capi CMakeLists is patched also.

comment:11 Changed 6 years ago by strk

Priority: majorblocker

@mat, do you have an estimate of when you'll be able to bring these in ? Should we wait for them before a final release ?

comment:12 Changed 6 years ago by Mateusz Łoskot

The supplementary tests.patch applied r3382

comment:13 Changed 6 years ago by Mateusz Łoskot

Applied William's patch with missing library versions for OSX framework (r3383)

comment:14 Changed 6 years ago by Mateusz Łoskot

Applied src_CMakeLists.txt.patch OSX framework (r3384) with slight modification:

  • this bit was originally placed in non-Apple branch, so moved outside:
    if(APPLE)
      set_target_properties(geos
        PROPERTIES
        BUILD_WITH_INSTALL_RPATH TRUE
        INSTALL_NAME_DIR "${CMAKE_INSTALL_PREFIX}")
    endif()
    

comment:15 in reply to:  14 Changed 6 years ago by Mateusz Łoskot

Applied capi_CMakeLists.txt.patch OSX framework (r3385) with similar modification as in r3384 above.

comment:16 Changed 6 years ago by Mateusz Łoskot

Resolution: fixed
Status: reopenedclosed

The SOVERSION support is marked as TODO in CMakeLists.txt. I also added new ticket #446.

I think it's OK to close this. Please, reopen if it still needs addition.

comment:17 Changed 6 years ago by kyngchaos

Resolution: fixed
Status: closedreopened

Sorry to beat this one...

The "if(APPLE AND GEOS_ENABLE_MACOSX_FRAMEWORK)" only gets the case of the framework build, so the the "else" can still be an OS X build, it's just a dylib. So those set_target_properties() are OK inside the else block. I guess it does no harm, since the same properties are set for framework and dylib builds.

For the capi, it should stay inside the "if(NOT APPLE AND NOT GEOS_ENABLE_MACOSX_FRAMEWORK)", because a OSX dylib build will be in there, and for a framework build the geos_c target will not exist. NOTE: it should be "set_target_properties(geos_c", I messed that one up.

comment:18 Changed 6 years ago by kyngchaos

Indeed, that must go inside the non-framework block - the framework target is "GEOS", the non-framework target is "geos", and cmake is case-sensitive for targets. A framework build fails configuration because target "geos" does not exist if that bit is outside the non-framework block.

comment:19 Changed 6 years ago by strk

Milestone: 3.3.03.3.1

Let's have this target 3.3.1 ...

comment:20 Changed 6 years ago by strk

@mloskot : planning to work on this for 3.3.1 ?

comment:21 Changed 6 years ago by strk

Milestone: 3.3.1GEOS Future
Priority: blockerminor

Doesn't look like this is happening anytime soon (unless someone provides a patch)

Changed 6 years ago by kyngchaos

Attachment: bug#385.patch added

comment:22 Changed 6 years ago by strk

Resolution: fixed
Status: reopenedclosed

Thanks. Committed as r3485.

Note: See TracTickets for help on using tickets.