Use Xcode as system generator and builder on OSX #127

esteve · 2016-12-08T10:01:06Z

This PR replaces make with xcodebuild as the builder for OSX, this among other things allows for ROS2 to be compiled for iOS and the iOS Simulator.

I kept the name of the builder flags option (make-flags), but I think that if this gets merged it may make sense to rename it to something else (e.g. builder-flags), since Linux would be the only platform to actually use make

esteve · 2017-03-22T11:30:14Z

@wjwwood sorry for requesting a review from you specifically, but given that you're the one who uses OSX to work on ROS2, I thought you'd the most appropriate for checking out this. However, if anyone else on the @ros2/team feels like having a look at the changes, please feel free to do so.

I believe the changes in this PR can be merged because they:

do not break existing code and all the existing usecases are supported (e.g. parallel building). IIRC last time I ran this on local, everything worked fine, hopefully nothing has changed.
add support for using ROS 2.0 as an XCode project (e.g. debugging from the IDE)
add support for building ROS 2.0 on iOS. The iOS SDK does not support Make AFAIK.

wjwwood · 2017-03-24T00:55:54Z

@esteve the changes look ok to me, but I would like to preserve the capability to build with make on macOS, something like a --use-xcode option for ament build. I'll try to take a stab at implementing this as an option as soon as I can.

wjwwood · 2017-03-24T00:58:27Z

Just for some of my thinking behind that desire... Xcode is not currently a requirement for building ROS 2 because you can just install "Command Line Tools" and avoid Xcode, which requires an apple account (I think that's still the case anyways). Also, I like to go into my build folders and run make ... 😄.

But still, the ability to run in Xcode is pretty nice. Luckily I don't think it will be difficult to make this switch optional.

esteve · 2017-03-24T09:34:22Z

@wjwwood probably bad phrasing on my part in my previous comment, but Xcode is not really a requirement for this. xcodebuild, which replaces make on this PR, is part of the "Command Line Tools" as well, so when you install Homebrew (which is a requirement for ROS 2.0), you already have it. So you can still build from the command line using either xcodebuild or cmake --build. I don't know if I still have permissions to trigger a CI build, but this change should still work on the buildfarm, which does not have Xcode installed.

Another option, if you want to keep the Make-based system is to check what generator was used (CMake's -G flag), and let ament choose make or xcodebuild accordingly.

esteve · 2017-03-24T13:26:38Z

@wjwwood what I mean is that, analogously to this code:

https://github.com/esteve/ament_tools/blob/xcode/ament_tools/build_types/cmake.py#L410-L435

something similar could be done in the build phase:

        elif IS_MACOSX:
            # Check CMake was invoked to generate a Xcode or Make project
            # The Xcode CMake generator will produce a file named
            # ALL_BUILD_cmakeRulesBuildPhase.makeRelease if the Xcode generator
            # was used, whether the build type is Release, Debug or anything else
            all_build_cmake_file_path = os.path.join(
                context.build_space, 'CMakeScripts', 'ALL_BUILD_cmakeRulesBuildPhase.makeRelease')
            all_build_cmake_file = os.path.isfile(all_build_cmake_file_path)
            if all_build_cmake_file:
                # This is an Xcode project, use xcodebuild to compile
                ...
                yield BuildAction(prefix + [XCODEBUILD_EXECUTABLE] + xcodebuild_args)
            else:
                # Assume that CMake generated a Make-based project
                ...
                yield BuildAction(prefix + [MAKE_EXECUTABLE] + context.make_flags)

and default to the Make generator. IMHO xcodebuild is better than Make (anything is better than Make 😄 ) and both are abstracted by cmake --build, but it's true that people are used to Make and it's hard to change habits, so if it causes less friction, I'm fine to set Make as the default type for the CMake generator.

What do you think? I can write that logic, but it'd be good to run a CI build with Xcode as the generator as well.

wjwwood · 2017-03-24T22:38:17Z

I don't actually mind what the default is on macOS (as long as it comes with CLT and doesn't introduce regressions), but I do think there should be a way to use make if you want, whether or not that's the default.

esteve · 2017-03-29T12:00:38Z

@wjwwood I've updated the PR to support both Make and Xcode, I've left Xcode as the default, but it can easily be changed to Make here:

https://github.com/esteve/ament_tools/blob/xcode/ament_tools/build_types/cmake.py#L171-L172

to override the default, the user can pass the -G flag to --cmake-args or --ament-cmake-args.

However, I had always tested with PR without symlink-install and I just noticed that it won't work because the Xcode generator is a multi-config generator (https://cmake.org/cmake/help/v3.5/variable/CMAKE_BUILD_TYPE.html#variable:CMAKE_BUILD_TYPE and https://cmake.org/cmake/help/v3.5/variable/CMAKE_CONFIGURATION_TYPES.html), and thus creates multiple conflicting targets if ament is asked to create symbolic links for the install step. I ended up adding the following to ament_cmake esteve/ament_cmake@9c27a52, so I don't know if it's just better to keep Make as the default generator/builder for CMake.

dirk-thomas · 2017-03-29T16:28:56Z

With the downside of not being able to use the symlink installs (not sure if that could be fixed, I don't have a platform with multi-config to test this on) I would propose to keep make the default.

to override the default, the user can pass the -G flag to --cmake-args or --ament-cmake-args.

I would suggest adding a option --use-xcode as it is done in ROS 1 for other generators already.

Please see #144 for a different PR adding support for Ninja which will likely interfere with this.

esteve · 2017-03-30T08:50:00Z

@dirk-thomas I agree, it makes sense. I've made the Make generator the default again (and more explicit see https://github.com/esteve/ament_tools/blob/xcode/ament_tools/build_types/cmake.py#L181) and added a --use-xcode option to ament which is only available when run on macOS.

Since it's not the default, I also added -DCMAKE_XCODE_GENERATE_SCHEME=ON, which will be available in CMake 3.8.0. The new scheme generator for CMake 3.8 adds better support for xcodebuild test, for example. This way, the default behavior will still be compatible with the minimum requirements (i.e. CMake 3.5) and only the optional --use-xcode flag requires a greater version of CMake.

As for the issue with multi-config generators and symlink installs, should I submit a PR with the change in esteve/ament_cmake@9c27a52 ?

dirk-thomas · 2017-03-30T12:43:10Z

As for the issue with multi-config generators and symlink installs, should I submit a PR with the change in esteve/ament_cmake@9c27a52 ?

It would be good to separate that change. I don't see why the symlinks install should need to be disabled. It is supposed to replace the normal install rules transparently - and those obviously work for multi configuration generators. So it looks like the symlinks install stuff needs to be modified somehow to work in that scenario.

esteve · 2017-03-30T13:02:46Z

and those obviously work for multi configuration generators.

@dirk-thomas this issue hadn't show up yet because the only multi config generator that's in use in ROS 2.0 is Visual Studio's and symlinks are not supported in Windows. However, macOS supports symlinks but the Xcode generator fails with the code here:

https://github.com/ament/ament_cmake/blob/master/ament_cmake_core/cmake/symlink_install/ament_cmake_symlink_install_targets.cmake#L96-L98

invoking ament with src/ament/ament_tools/scripts/ament.py build --use-xcode --build-tests --symlink-install --isolated --build-space build_isolated_xcode --install-space install_isolated_xcode produces the following error:

+++ Building 'class_loader'
==> '. /Users/esteve/ros2_ws/build_isolated_xcode/class_loader/cmake__build.sh && /usr/local/bin/cmake /Users/esteve/ros2_ws/src/ros/class_loader -DBUILD_TESTING=1 -DAMENT_CMAKE_SYMLINK_INSTALL=1 -DCMAKE_INSTALL_PREFIX=/Users/esteve/ros2_ws/install_isolated_xcode/class_loader -G Xcode -DCMAKE_XCODE_GENERATE_SCHEME=ON' in '/Users/esteve/ros2_ws/build_isolated_xcode/class_loader'
-- Found ament_cmake: 0.0.0 (/Users/esteve/ros2_ws/install_isolated_xcode/ament_cmake/share/ament_cmake/cmake)
-- Using PYTHON_EXECUTABLE: /usr/local/bin/python3
-- Override CMake install command with custom implementation using symlinks instead of copying resources
-- Found poco_vendor: 1.0.0 (/Users/esteve/ros2_ws/install_isolated_xcode/poco_vendor/share/poco_vendor/cmake)
-- Searching for Poco library...
--   Found Poco!
-- components found: Foundation.
-- Configuring done
CMake Error in CMakeLists.txt:
  Evaluation file to be written multiple times for different configurations
  or languages with different content:

    /Users/esteve/ros2_ws/build_isolated_xcode/class_loader/ament_cmake_symlink_install_targets_0.cmake


CMake Error in CMakeLists.txt:
  Evaluation file to be written multiple times for different configurations
  or languages with different content:

    /Users/esteve/ros2_ws/build_isolated_xcode/class_loader/ament_cmake_symlink_install_targets_0.cmake


CMake Error in CMakeLists.txt:
  Evaluation file to be written multiple times for different configurations
  or languages with different content:

    /Users/esteve/ros2_ws/build_isolated_xcode/class_loader/ament_cmake_symlink_install_targets_0.cmake


CMake Error in CMakeLists.txt:
  Evaluation file to be written multiple times for different configurations
  or languages with different content:

    /Users/esteve/ros2_ws/build_isolated_xcode/class_loader/ament_cmake_symlink_install_targets_0.cmake


CMake Error in CMakeLists.txt:
  Evaluation file to be written multiple times for different configurations
  or languages with different content:

    /Users/esteve/ros2_ws/build_isolated_xcode/class_loader/ament_cmake_symlink_install_targets_0.cmake


CMake Error in CMakeLists.txt:
  Evaluation file to be written multiple times for different configurations
  or languages with different content:

    /Users/esteve/ros2_ws/build_isolated_xcode/class_loader/ament_cmake_symlink_install_targets_0.cmake


-- Generating done
-- Build files have been written to: /Users/esteve/ros2_ws/build_isolated_xcode/class_loader

<== Command '. /Users/esteve/ros2_ws/build_isolated_xcode/class_loader/cmake__build.sh && /usr/local/bin/cmake /Users/esteve/ros2_ws/src/ros/class_loader -DBUILD_TESTING=1 -DAMENT_CMAKE_SYMLINK_INSTALL=1 -DCMAKE_INSTALL_PREFIX=/Users/esteve/ros2_ws/install_isolated_xcode/class_loader -G Xcode -DCMAKE_XCODE_GENERATE_SCHEME=ON' failed in '/Users/esteve/ros2_ws/build_isolated_xcode/class_loader' with exit code '1'
<== Command '. /Users/esteve/ros2_ws/build_isolated_xcode/class_loader/cmake__build.sh && /usr/local/bin/cmake /Users/esteve/ros2_ws/src/ros/class_loader -DBUILD_TESTING=1 -DAMENT_CMAKE_SYMLINK_INSTALL=1 -DCMAKE_INSTALL_PREFIX=/Users/esteve/ros2_ws/install_isolated_xcode/class_loader -G Xcode -DCMAKE_XCODE_GENERATE_SCHEME=ON' failed in '/Users/esteve/ros2_ws/build_isolated_xcode/class_loader' with exit code '1'

dirk-thomas · 2017-03-30T20:23:44Z

ament_tools/build_types/cmake.py

+        elif IS_MACOSX:
+            if self._using_xcode_generator(context):
+                if XCODEBUILD_EXECUTABLE is None:
+                    raise VerbExecutionError("Could not find 'xcodebuild' executable")


Does this prevent building with Makefiles when xcodebuild is not available?

Just based on the code, I think this only happens when you've specified that you want to use the xcode generator.

dirk-thomas · 2017-03-30T20:23:46Z

ament_tools/build_types/cmake.py

+                                flag.replace(
+                                    '-j', '-IDEBuildOperationMaxNumberOfConcurrentCompileTasks='))
+                        else:
+                            xcodebuild_flags.append(flag)


The logic is a bit difficult to follow. Shouldn't this be the equivalent of:

if flag.startswith('-j'): xcodebuild_flags.append(flag.replace( '-j', '-IDEBuildOperationMaxNumberOfConcurrentCompileTasks=')) elif not flag.startswith('-l'): xcodebuild_flags.append(flag)

👍, the altered logic is easier to read I think.

dirk-thomas · 2017-03-30T20:26:11Z

ament_tools/build_types/cmake.py

+                                xcodebuild_flags.append(flag)
+                    xcodebuild_flags += ['-configuration']
+                    xcodebuild_flags += [self._get_configuration_from_cmake(context)]
+                    cmd.extend(xcodebuild_flags)


Almost the logic has been added above. This should be deduplicated.

Another time below.

I agree that some of this logic could be consolidated with the code above. Especially the handling of the context.make_flags.

Agreed, I've added @dirk-thomas 's code and moved it to a separate function in 9ace6e8

Thanks!

Update: I removed the duplicated code since it was not needed and simplified the logic for flag processing in 1974198

wjwwood

This is really close I think. There were a few requests to simplify logic and consolidate code. Once those are done and we test/merge @dirk-thomas change to support multiple build types with symlink install, then I'll spend some time testing this out locally on my own machine.

Thanks @esteve for iterating with us.

wjwwood · 2017-03-30T22:22:09Z

ament_tools/build_types/cmake.py

+            elif IS_MACOSX:
+                if context.use_xcode or self._using_xcode_generator(context):
+                    cmake_args += ['-G', 'Xcode']
+                    cmake_args += ['-DCMAKE_XCODE_GENERATE_SCHEME=ON']


This one assumes CMake 3.8.0 right? Could we conditionally add that with that version only? Or is there no point in running without it? My impression was that it just improved testing with Xcode?

I've removed the CMAKE_XCODE_GENERATE_SCHEME flag and used the RUN_TESTS target instead for ament test as a workaround in eaf9430 This approach is compatible with CMake 3.5, so no need to use anything experimental from 3.8

Update: the latest commit id with the change is a39cd8c

wjwwood · 2017-03-30T22:23:56Z

ament_tools/build_types/cmake.py

+                                flag.replace(
+                                    '-j', '-IDEBuildOperationMaxNumberOfConcurrentCompileTasks='))
+                        else:
+                            xcodebuild_flags.append(flag)


👍, the altered logic is easier to read I think.

wjwwood · 2017-03-30T22:29:53Z

ament_tools/build_types/cmake.py

+                                xcodebuild_flags.append(flag)
+                    xcodebuild_flags += ['-configuration']
+                    xcodebuild_flags += [self._get_configuration_from_cmake(context)]
+                    cmd.extend(xcodebuild_flags)


I agree that some of this logic could be consolidated with the code above. Especially the handling of the context.make_flags.

esteve · 2017-04-07T10:34:20Z

@dirk-thomas @wjwwood I've addressed your feedback, let me know if there's anything else that needs to be fixed/improved. Thanks!

…GENERATE_SCHEME

esteve · 2017-04-07T11:00:00Z

@dirk-thomas @wjwwood I just realized that the duplicated code was not really needed in the install and uninstall verbs, so I moved the code back to _common_cmake_on_build

dirk-thomas · 2017-04-07T16:35:48Z

Not directly related to this patch but a lot of this custom logic would be unnecessary if we would implement #88. But since ament_tools is likely not being developed much further (see http://design.ros2.org/articles/build_tool.html) I don't think we will spend time to migrate to cmake --build. Since this patch addresses the short term desire to use XCode that's fine with me.

I will leave it up to @wjwwood to try it on OS X and comment / merge this patch.

wjwwood · 2017-04-07T23:06:11Z

I tested the --use-xcode option locally and it works!

I also ran CI with the default behavior to see if breaks anything, but it works (after 0371b58):

Linux:
macOS:
- (flaky tests make it unstable)
Windows:
- (flaky tests, compared to similar nightly failures)

But of course just as I get it tested, it's all broken now that @dirk-thomas pushed #144 through.

I'll have a look at the conflict resolution...

wjwwood · 2017-04-08T01:20:18Z

Passing CI:

Linux:
macOS:
- (with some flaky tests)
Windows:
- (also with some flaky tests)

wjwwood · 2017-04-08T01:20:32Z

Thanks again @esteve!

esteve · 2017-04-10T10:29:27Z

Yay! Thanks @wjwwood for testing this and for fixing the conflicts.

esteve · 2017-04-10T10:29:38Z

And thanks @dirk-thomas for the review.

This reverts commit 2ed6995.

esteve added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 8, 2016

esteve requested a review from wjwwood March 22, 2017 11:21

esteve added 2 commits March 29, 2017 13:41

Use Xcode as system generator and builder on OSX

c00bbe9

Enable building with either Xcode or Make on macOS

8202be6

dirk-thomas mentioned this pull request Mar 29, 2017

Added support for ninja build system #144

Merged

Added --use-xcode option to ament

a2f7bc1

dirk-thomas mentioned this pull request Mar 30, 2017

support symlink install for config specific targets ament/ament_cmake#89

Merged

dirk-thomas reviewed Mar 30, 2017

View reviewed changes

wjwwood suggested changes Mar 30, 2017

View reviewed changes

mikaelarguedas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Apr 5, 2017

esteve added 2 commits April 7, 2017 12:56

Simplified logic for flag processing

1974198

Use RUN_TESTS target for testing instead of depending on CMAKE_XCODE_…

a39cd8c

…GENERATE_SCHEME

fix access of option thats only on macOS

0371b58

Merge branch 'master' into xcode

1c44820

wjwwood approved these changes Apr 7, 2017

View reviewed changes

fix conflict resolution bug

08aceb4

wjwwood merged commit 2ed6995 into ament:master Apr 8, 2017

wjwwood removed the in progress Actively being worked on (Kanban column) label Apr 8, 2017

esteve deleted the xcode branch April 10, 2017 10:28

dhood added a commit that referenced this pull request May 1, 2017

Revert "Use Xcode as system generator and builder on OSX (#127)"

8c10929

This reverts commit 2ed6995.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Xcode as system generator and builder on OSX #127

Use Xcode as system generator and builder on OSX #127

esteve commented Dec 8, 2016

esteve commented Mar 22, 2017 •

edited by wjwwood

Loading

wjwwood commented Mar 24, 2017

wjwwood commented Mar 24, 2017

esteve commented Mar 24, 2017

esteve commented Mar 24, 2017 •

edited

Loading

wjwwood commented Mar 24, 2017

esteve commented Mar 29, 2017

dirk-thomas commented Mar 29, 2017

esteve commented Mar 30, 2017

dirk-thomas commented Mar 30, 2017

esteve commented Mar 30, 2017

dirk-thomas Mar 30, 2017

wjwwood Mar 30, 2017

dirk-thomas Mar 30, 2017

wjwwood Mar 30, 2017

dirk-thomas Mar 30, 2017

wjwwood Mar 30, 2017

esteve Apr 7, 2017 •

edited

Loading

wjwwood left a comment

wjwwood Mar 30, 2017

esteve Apr 7, 2017 •

edited

Loading

wjwwood Mar 30, 2017

wjwwood Mar 30, 2017

esteve commented Apr 7, 2017

esteve commented Apr 7, 2017

dirk-thomas commented Apr 7, 2017

wjwwood commented Apr 7, 2017

wjwwood commented Apr 8, 2017

wjwwood commented Apr 8, 2017

esteve commented Apr 10, 2017

esteve commented Apr 10, 2017

Use Xcode as system generator and builder on OSX #127

Use Xcode as system generator and builder on OSX #127

Conversation

esteve commented Dec 8, 2016

esteve commented Mar 22, 2017 • edited by wjwwood Loading

wjwwood commented Mar 24, 2017

wjwwood commented Mar 24, 2017

esteve commented Mar 24, 2017

esteve commented Mar 24, 2017 • edited Loading

wjwwood commented Mar 24, 2017

esteve commented Mar 29, 2017

dirk-thomas commented Mar 29, 2017

esteve commented Mar 30, 2017

dirk-thomas commented Mar 30, 2017

esteve commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esteve Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esteve Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esteve commented Apr 7, 2017

esteve commented Apr 7, 2017

dirk-thomas commented Apr 7, 2017

wjwwood commented Apr 7, 2017

wjwwood commented Apr 8, 2017

wjwwood commented Apr 8, 2017

esteve commented Apr 10, 2017

esteve commented Apr 10, 2017

esteve commented Mar 22, 2017 •

edited by wjwwood

Loading

esteve commented Mar 24, 2017 •

edited

Loading

esteve Apr 7, 2017 •

edited

Loading

esteve Apr 7, 2017 •

edited

Loading