colcon / colcon-cmake

Extension for colcon to support CMake packages
http://colcon.readthedocs.io
Apache License 2.0
16 stars 25 forks source link

Option to select config for mulit-configuration-generator #48

Closed KazNX closed 4 years ago

KazNX commented 4 years ago

Hi,

I'd like to propose an extension to colcon-cmake build arguments to support selecting the build configuration for a multi-configuration-generator, such as Visual Studio. I have a relatively simple change proposed below, nI'm looking for comments on the wider implications of this change within the colcon ecosystem.

Currently, when colcon-cmake build.py invokes CMake, it appends --config CONFIG based on the current CMAKE_BUILD_TYPE, defaulting to "Release" if none is specified. The change I propose is to allow a command line selection of this value.

In CmakeBuildTask.add_arguments(), add a definition for a --config CONFIG argument:

        parser.add_argument(
            '--config',
            help='Select the CMake build configuration: [Debug, MinSizeRel, Release, RelWithDebInfo]. Only for multi configuration generators (e.g., Visual Studio)')

Modify CmakeBuildTask._get_configuration() as follows. Changes are highighted between # @@@ Change begin and # @@@ Change end comments.

    def _get_configuration(self, args):
        # check for CMake build type in the command line arguments
        arg_prefix = '-DCMAKE_BUILD_TYPE='
        build_type = None
        for cmake_arg in (args.cmake_args or []):
            if cmake_arg.startswith(arg_prefix):
                build_type = cmake_arg[len(arg_prefix):]
        if build_type is None:
            # get the CMake build type from the CMake cache
            build_type = get_variable_from_cmake_cache(
                args.build_base, 'CMAKE_BUILD_TYPE')
        if build_type in ('Debug', 'MinSizeRel', 'RelWithDebInfo'):
            return build_type

        # @@@ Change begin
        # Select build configuration for multi-configuration generators if the
        # --config option is presend on the command line
        multi_configuration_generator = is_multi_configuration_generator(
            args.build_base, args.cmake_args)
        if multi_configuration_generator and args.config:
            return args.config
        # @@@ Change end
        return 'Release'

This works in my isolated, CMake only build tree, but I suspect I'm missing something similar for running in test mode. Also, this may not play nice with other build tasks, such as ament.

Comments welcome.

dirk-thomas commented 4 years ago

I am not sure I understand the purpose of the new option --config. What is the advantage over passing CMAKE_BUILD_TYPE as it is supported already?

KazNX commented 4 years ago

Depenending on how the CMake config is set up, building and installing debug only targets can result in find_package() scripts marking the package as not found because it can't find a release library.

This may be a bit hard to follow, but in my use case with just CMAKE_BUILD_TYPE=Debug a third party package exports a <package>-targets-debug.config, but fails to identify a release target, so <package>_LIBRARY ends up undefined. This reuslts in <package>_FOUND being FALSE. find_package(<package>) then fails. This issue comes about partly due to a mixed usage of <package>_LIBRARY and CMake import targets, but neither approach is ubuquitous.

With the change above, I can build both release (must come first) and debug into the same install tree and avoid this issue. In my opinion, this is a better fit for Visual Studio/Windows compilation.

As an asside, I really approciate being able to use colcon to globally ensure cmake is passed -DCMAKE_DEBUG_POSTFIX=d to ensure debug libraries are distinctly named at a global level.

KazNX commented 4 years ago

Also, I can open the Visual Studio sln files directly and build specific projects in either debug or release and reduce file overhead etc.

dirk-thomas commented 4 years ago

in my use case with just CMAKE_BUILD_TYPE=Debug a third party package exports a -targets-debug.config, but fails to identify a release target, so _LIBRARY ends up undefined.

How would that be any different when using --config instead of CMAKE_BUILD_TYPE?

In general you can't mix debug and release artifacts on Windows since they are ABI incompatible.

With the change above, I can build both release (must come first) and debug into the same install tree and avoid this issue.

Why can't you achieve the same with two builds using different CMAKE_BUILD_TYPE?

I can open the Visual Studio sln files directly and build specific projects in either debug or release and reduce file overhead etc.

Same question here: you can already do that when using CMAKE_BUILD_TYPE, no?

KazNX commented 4 years ago

How would that be any different when using --config instead of CMAKE_BUILD_TYPE?

If export targets are properly used by the CMake config (which in this case they are), then CMake produces a number of files to aid in finding and importing the package. Specifically:

This is the connonical way of supporting debug and release targets. However, using CMAKE_BUILD_TYPE=Debug the file <package>-targets-release.cmake is not produced since there is no release library. This results in find_pacakge(<package>) failing to set <package>_FOUND.

In general you can't mix debug and release artifacts on Windows since they are ABI incompatible.

True and I'm not trying to. I'm trying to output multiple configurations into the same artefact tree, supported by the <package>-targets-<config>.cmake setup described above. For libraries which don't explictly support this, I'm using -DCMAKE_DEBUG_POSTFIX to ensure debug artefacts have distinct names.

Why can't you achieve the same with two builds using different CMAKE_BUILD_TYPE

Refer to the first answer regarding failing to resolve a package as found without a release library.

I'm working on a project setup to illustrate this situation.

dirk-thomas commented 4 years ago

This is the connonical way of supporting debug and release targets.

That still doesn't answer my question why you can't use CMAKE_BUILD_TYPE the same way as the proposed --config option.

Please describe a sequence of steps which work with the new option but where a similar sequence using the build type variable doesn't work.

KazNX commented 4 years ago

So it turns out that the 3rd-party project I was using was doing unusual things to resolve 3rd-party libraries and couldn't find the debug only library, which mean using CMAKE_BUILD_TYPE=Debug would fail. This makes my original premise moot and the direct answer to your question is that I actually can use CMAKE_BUILD_TYPE=Debug so long as I fix the 3rd-party CMake setup.

Having said that, this situation really just prompted me to explore whether it was possible to use colcon to select the build type for multi-configuration generators such as Visual Studio or Xcode. With that in mind, can I'd like to turn the question around at this point; is there a good reason not to support my propose change?

dirk-thomas commented 4 years ago

With that in mind, can I'd like to turn the question around at this point; is there a good reason not to support my propose change?

Yes, there is already a way to choose the build type - using CMAKE_BUILD_TYPE. Adding a second option to do the very same thing seems unnecessary and would need to be maintained in the future.

KazNX commented 4 years ago

I can see the maintenance point. Ultimately, I was hoping to be able to setup my project to build all the 3rdparty stuff and configure my leaf project, then use one Visual Studio solution directly to build both release and debug.

dirk-thomas commented 4 years ago

I was hoping to be able to setup my project to build all the 3rdparty stuff and configure my leaf project, then use one Visual Studio solution directly to build both release and debug.

I don't see why you wouldn't be able to do that using the CMAKE_BUILD_TYPE?

KazNX commented 4 years ago

So you've given me pause for thought. I've had to rewind a bit to work out why I wasn't heading in that direction. It comes down to how I've set up my mixins and working on multiple platforms.

I have a debug mixin which defines different install/build to support side-by-side release/debug builds on Linux (using make/ninja). I was using the same mixing for windows. Removing this mixin CMAKE_BUILD_TYPE is enough.

To support both Windows and Linux configurations, I need to either different mixin names (I was aiming for consistent build command lines) or load mixins from different locations. Note: I'm using COLCON_HOME=./.colcon to make all the mixin configuration local to the project.

Mixins don't happen to support OS specific sections do they? That would certainly solve this issue.

Otherwise, I'm going to ponder how how else I can get the cross-platform support I'm hoping for.

Thanks for the help.

dirk-thomas commented 4 years ago

Mixins don't happen to support OS specific sections do they?

No, there is no OS specific handling for mixins at the moment. That would be interesting feature idea though...

KazNX commented 4 years ago

I think we can put the original question to bed now. Thanks for the perspective.

Having used colcon in earnest, two feature requests come to mind:

  1. Support for OS/compiler specific tags in various configuration files as suggested above. In approximate order of importance for this support: *.mixin files, defaults.yaml, colcon.meta, colcon.pkg
  2. Support for multiple search locations for defaults.yaml and *.mixin files and/or searching under ./.colcon first instead of ~/.colcon.

For point 1. I think OS specific blocks would be extremely powerful and mean the 90% case. I explicitly mention compiler sepcific tags as a more general solution because MinGW behaves very differently to the Microsoft compiler. I anticipate this would be harder had provide less bang for buck.

Point 2. is more minor as I am currently setting COLCON_HOME to be ./.colcon to get the behaviour I need and support package local configuration. However, it's an additional setup step to ensure the wider team I work with get right and it's an easy one to miss. Thus I'd prefer to have this behaviour be implicit.

dirk-thomas commented 4 years ago

Support for OS/compiler specific tags in various configuration files as suggested above. In approximate order of importance for this support: *.mixin files, defaults.yaml, colcon.meta, colcon.pkg

I am not sure if platform / environment specific tags (input) is sufficient. The yaml files are too static to use such input. A concept to select alternating blocks depending of the input / arbitrary conditions would be necessary imo to make this useful.

Support for multiple search locations for defaults.yaml and *.mixin files and/or searching under ./.colcon first instead of ~/.colcon.

While a list of paths to search would be easily to realize the question is how is the resolution order. Only pick the first one (implying that a workspace specific file overrides a user specific file - probably not always intended)? Or consider all found ones and somehow combine then (what does it mean to combined them for the various different type of configuration options)?

Also related is colcon/colcon-core#168.

dirk-thomas commented 4 years ago

Assume the conversation has resolved the original question I am going ahead and close the ticket. Please feel free to continue commenting and if necessary the ticket can be reopened.

talregev commented 4 years ago

I was hoping to be able to setup my project to build all the 3rdparty stuff and configure my leaf project, then use one Visual Studio solution directly to build both release and debug.

I don't see why you wouldn't be able to do that using the CMAKE_BUILD_TYPE?

It not the same. -DCMAKE_BUILD_TYPE=Release is on the configure time of the cmake package. and cmake --build {DIR} --config Release It on the build time.

You can see it more clearly when you try to build on windows with visual studio If you do -DCMAKE_BUILD_TYPE=Release The colcon still can build the package with debug mode. and search for debug lib files instead the release.
even with this option: -DCMAKE_BUILD_TYPE=Release
It happen to me, and it difficult to change that.

I succeeded to build in release mode with cmake --build . --config Release But not with colcon. Can you make this option available?

You can read it more here: https://stackoverflow.com/questions/19024259/how-to-change-the-build-type-to-release-mode-in-cmake/20423820#20423820

dirk-thomas commented 4 years ago

@talregev This ticket is already closed for five months. I suggest if you have a similar problem to open a new ticket since comments like this are likely being lost.

I succeeded to build in release mode with cmake --build . --config Release But not with colcon. Can you make this option available?

You can read it more here: https://stackoverflow.com/questions/19024259/how-to-change-the-build-type-to-release-mode-in-cmake/20423820#20423820

Please consider to contribute a pull request if you don't want to simply pass a different -DCMAKE_BUILD_TYPE= in a second invocation which should achieve the very same thing.

KazNX commented 4 years ago

@talregev I also suggest you open a new ticket. I have been generally getting away with the current behaviour for a while now without major issues. I suspect there are some specific cases where the current behaviour will fail.

talregev commented 4 years ago

Thank you for reading my comment. The issue was resolved for me when I put: colcon build --cmake-args -DCMAKE_BUILD_TYPE=Release then colcon run: cmake --build . --config Release.