Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for just configuring cmake projects #119

Closed
wants to merge 2 commits into from

Conversation

kunaltyagi
Copy link

Fixes #116

  1. Made the new argument the penultimate one so as to not interfere with other PRs
  2. Don't really know how to handle rc
  3. Put the log messages after identifying the project name (can also not use it, but it felt proper to use the name)

@kunaltyagi
Copy link
Author

@dirk-thomas PTAL

Can we run the CI at least in the meantime?

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks!

@cottsay cottsay self-requested a review June 1, 2022 19:54
@kunaltyagi
Copy link
Author

Oh yeah, completely forgot I made this PR 😆 and felt the need again. Would there be any progress on this? 😅

@dirk-thomas
Copy link
Member

@kunaltyagi Sorry, I am not a maintainer of this repository anymore.

@kunaltyagi
Copy link
Author

You've been pivotal in a ton of projects and it's been a privilege using them.

Paging @cottsay (hope this time I get it correct 😂) instead

@imcelroy
Copy link

@kunaltyagi @cottsay Any news about this? This feature would be a great addition!

@kunaltyagi
Copy link
Author

Seems like Scott is quite busy (elsewhere). Lack of activity in a while makes me think this PR wouldn't get merged in a hurry.

That's a bit sad since CMake is a key builder for ROS2.

@cottsay
Copy link
Member

cottsay commented Oct 12, 2022

Please take a look at #124.

I'm trying to avoid adding new arguments for such a narrow use case, so I'm proposing we expand the capability of an existing one to support this scenario.

@kunaltyagi
Copy link
Author

That PR seems to be orthogonal. If there are no cmake-targets, it performs install, and if there are cmake-targets, it builds them.

This PR is to stop cmake right after configuration completes, before building anything.

@cottsay
Copy link
Member

cottsay commented Oct 13, 2022

If there are no cmake-targets, it performs install

This is not true in #124, and was not true with the previous behavior either. Manually specifying targets (including no targets) suppresses the default additional install invocation.

@kunaltyagi
Copy link
Author

So, in order to just configure, not build, the invocation would be:

colcon build --cmake-targets

?

@cottsay
Copy link
Member

cottsay commented Oct 16, 2022

So, in order to just configure, not build, the invocation would be:

colcon build --cmake-targets

That's correct. Normal arguments could appear after --cmake-targets, e.x. colcon build --cmake-targets --event-handler console_direct+. In mixins, defaults files, and meta files, it would look like this:

  ...
  "cmake_targets": [],
  ...

As stated in the PR, the goal is to allow more than one custom target to be invoked, but it was trivial to support this scenario under that feature as well, without adding an additional command line argument to the already overwhelming list of possible arguments to colcon build.

If you're looking for something that explicitly references only configuring, we could create a mixin that specifies the empty target list so that you could invoke the build with --mixin configure-only or something like that.

@kunaltyagi
Copy link
Author

Perfect :)

@kunaltyagi kunaltyagi closed this Oct 17, 2022
@kunaltyagi kunaltyagi deleted the patch-1 branch October 17, 2022 03:39
@cottsay
Copy link
Member

cottsay commented Oct 17, 2022

Perfect :)

Thanks for your consideration, @kunaltyagi, and for pushing this issue forward. It would be helpful if you could provide feedback on #124, especially if you'd like to see it merged swiftly.

@kunaltyagi
Copy link
Author

  • add more info to the help regarding the difference between no --cmake-targets and a blank --cmake-targets.
  • make a note in code not to add "append" as action because that'd change the desired behavior
    rest lgtm

@okvik
Copy link

okvik commented Nov 16, 2022

I have this feature implemented in my own copy of colcon-cmake and find it indispensable. In particular, for having cmake generate compile-commands.json for easy browsing of huge source sets without having to build them.

IMO the proposed reuse of cmake-targets is arcane UI that mixes up the stages of regular cmake usage. Having to document this oddity and adding a mixin to try and hide it's arcanity does worse for the volume of colcon build --help than just adding a straightforward self-documenting flag. </bikeshedding>

@xcy-megvii
Copy link

@okvik How does this work? Without a build, cmake would not generate xxxConfig.cmake for a package, and those packages who depend on it would fail to find dependencies when configuring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add --cmake-only-configure option
7 participants