Ticket #385 (closed enhancement: fixed)

Opened 3 years ago

Last modified 20 months ago

Add OS X framework build option to cmake

Reported by: kyngchaos Owned by: mloskot
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

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

Change History

Changed 3 years ago by kyngchaos

Changed 3 years ago by kyngchaos

info.plist template, goes in src/

  Changed 2 years ago by strk

  • owner changed from pramsey to mloskot

  Changed 2 years ago by mloskot

  • status changed from new to assigned

  Changed 2 years ago by mloskot

  • keywords cmake added
  • status changed from assigned to closed
  • resolution set to fixed

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

  Changed 2 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?

  Changed 2 years ago by strk

  • status changed from closed to reopened
  • resolution fixed deleted

  Changed 2 years ago by strk

  • status changed from reopened to closed
  • resolution set to fixed

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..

  Changed 2 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 2 years ago by kyngchaos

supplementary patch for tests

  Changed 2 years ago by kyngchaos

  • status changed from closed to reopened
  • resolution fixed deleted

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

Changed 2 years ago by kyngchaos

patch to add missing library versions to framework

  Changed 2 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 2 years ago by kyngchaos

install_name_dir fix

Changed 2 years ago by kyngchaos

install_name_dir fix

  Changed 2 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.

  Changed 2 years ago by strk

  • priority changed from major to blocker

@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 ?

  Changed 2 years ago by mloskot

The supplementary tests.patch applied r3382

  Changed 2 years ago by mloskot

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

follow-up: ↓ 15   Changed 2 years ago by mloskot

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()

in reply to: ↑ 14   Changed 2 years ago by mloskot

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

  Changed 2 years ago by mloskot

  • status changed from reopened to closed
  • resolution set to fixed

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.

  Changed 2 years ago by kyngchaos

  • status changed from closed to reopened
  • resolution fixed deleted

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.

  Changed 2 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.

  Changed 2 years ago by strk

  • milestone changed from 3.3.0 to 3.3.1

Let's have this target 3.3.1 ...

  Changed 22 months ago by strk

@mloskot : planning to work on this for 3.3.1 ?

  Changed 20 months ago by strk

  • priority changed from blocker to minor
  • milestone changed from 3.3.1 to GEOS Future

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

Changed 20 months ago by kyngchaos

  Changed 20 months ago by strk

  • status changed from reopened to closed
  • resolution set to fixed

Thanks. Committed as r3485.

Note: See TracTickets for help on using tickets.