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

Workaround to exclude Clion's cmake folders from colcon build and tests #410

Merged

Conversation

MichaelOrlov
Copy link
Contributor

  • Add AMENT_IGNORE to CMAKE_BINARY_DIR to avoid picking up cmake specific folders created by CLion in colcon build and colcon test commands

Signed-off-by: Michael Orlov michael.orlov@apex.ai

- Add AMENT_IGNORE to CMAKE_BINARY_DIR to avoid picking up cmake
specific folders created by CLion in `colcon build` and `colcon test`
commands

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov MichaelOrlov marked this pull request as ready for review November 3, 2022 04:31
@MichaelOrlov
Copy link
Contributor Author

@wjwwood @clalancette Please review.

@mjcarroll
Copy link
Contributor

I'm curious why this is necessary as colcon already creates build/COLCON_IGNORE as well as install/COLCON_IGNORE and log/COLCON_IGNORE. Is CLion doing something different?

@MichaelOrlov
Copy link
Contributor Author

@mjcarroll Yes. CLion creating it's own folders for build.
Basically this fix adding AMENT_IGNORE to the place highlighted on screenshot.
image

@wjwwood
Copy link
Contributor

wjwwood commented Nov 3, 2022

created by CLion in colcon build and colcon test commands

This is confusing to me, can you describe your workflow better? Are you using CLion only or colcon only or both?

@MichaelOrlov
Copy link
Contributor Author

@wjwwood My workflow is that I use both CLion build for particular package running above build from colcon to be able to use debugger and run unit tests from CLion. As well as using colcon build and colcon test commands separately.

The problem is as soon as I start CLion and load some CMake file from any ROS2 package CLion will create set of sub-folders cmake-build-debug, cmake-build-relwithdebinfo etc. Those sub-folders needed for CLion to be able properly function. It keeps there some indexes and pulling out dependencies when loading CMake file to be able to parse project and highlights with smart code navigation.

The problems starts when I try to call regular colcon build or colcon test. Colcon will try to build and test code from those cmake-build-debug, cmake-build-relwithdebinfo as well as run linters on them. I ma getting a bunch of errors.
As workaround I have to delete all such folder from CLion when need to use colcon - but it's very cumbersome since I am opening and then closing multiple packages and some times difficult to find where else leftovers from CLion.

Proposed workaround will automatically put AMENT_IGNORE file when CLion will automatically create such folders. i.e. colcon will ignore such folders and everyone will be happy.

I have been using this fix for a couple years for development for ROS2 and we are using it in ApexAI internally for more then 3 years. It should be pretty much safe and I don't envision any side effects.

@sloretz
Copy link
Contributor

sloretz commented Nov 3, 2022

I can't think of anything that will break with this workaround. Maybe it would break someone wanting to do in-source builds, but I wouldn't expect those to be supported by Colcon anyways.

The problem is as soon as I start CLion and load some CMake file from any ROS2 package CLion will create set of sub-folders cmake-build-debug, cmake-build-relwithdebinfo etc. Those sub-folders needed for CLion to be able properly function. It keeps there some indexes and pulling out dependencies when loading CMake file to be able to parse project and highlights with smart code navigation.

Maybe there's a setting that will make CLion do that work out-of-source? https://intellij-support.jetbrains.com/hc/en-us/community/posts/360007742199-How-to-set-a-default-build-directory-OUTSIDE-the-source-directory-

@MichaelOrlov
Copy link
Contributor Author

@sloretz As regards to the

Maybe it would break someone wanting to do in-source builds

Could you please clarify how it could break and what exactly?

As regards to the

Maybe there's a setting that will make CLion do that work out-of-source? https://intellij-support.jetbrains.com/hc/en-us/community/posts/360007742199-How-to-set-a-default-build-directory-OUTSIDE-the-source-directory-

This is more question in jetbrain support system rather then answer on the problem and pretty much outdated. There are no such options File | Other Settings | Settings for New Projects…. in CLion any more. At least in 2021.1.3 version.

@clydemcqueen
Copy link

Another workaround: you can add a "." prefix to the various cmake-build-* directories to hide them from colcon.

@MichaelOrlov
Copy link
Contributor Author

@clydemcqueen Unfortunately not. We can't add "." prefix to the various cmake-build-* directories since those folder names manged by the CLion directly and not available for users to be changed.

The purpose of this PR is to provide seamless integration of the colcon with CLion IDE for final users rather than searching for some workarounds about which most users will not be aware.

@wjwwood
Copy link
Contributor

wjwwood commented Nov 28, 2022

I'm not a maintainer on this repository, so I'll leave it to @mjeronimo and/or previous maintainers (maybe @hidmic has an opinion).

For what it's worth I think that this patch is unlikely to cause a problem and we should probably merge it. But also, I do not think this is the right fix from ament/colcon's perspective. Ideally the src folder would be read-only for build tools (and introspection tools like clion) and therefore finding a way to make clion not create temporary files in your source code folder would be the best solution.

@sloretz
Copy link
Contributor

sloretz commented Nov 28, 2022

I'm also not a maintainer, but given this is an issue with the combination of colcon and CLion I'd recommend solving it by creating a colcon-clion plugin that automatically ignored these folders. That would solve both packages using ament_cmake, and plain CMake packages in one go.

@mjeronimo
Copy link
Contributor

I think that the proposed change is a reasonable workaround. Adding a COLCON_IGNORE to the CMAKE_BINARY_DIR seems fairly straightforward since colcon shouldn't be building anything there anyways. Also, having the IGNORE file there is also clear about the intention (versus having a plugin filter it out, which could be harder to debug if there was a problem). Seems OK to me.

@wjwwood
Copy link
Contributor

wjwwood commented Nov 28, 2022

I would recommend AMENT_IGNORE (which is what is in the pr now) over COLCON_IGNORE.

@mjeronimo
Copy link
Contributor

Yes, agreed. AMENT_IGNORE

@mjeronimo
Copy link
Contributor

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@mjeronimo
Copy link
Contributor

Re-running the Windows build:

  • Windows Build Status

@mjeronimo mjeronimo merged commit d38014a into ament:rolling Nov 29, 2022
@MichaelOrlov
Copy link
Contributor Author

Thank you folks for accepting and merging this PR.

@alsora
Copy link
Contributor

alsora commented Jun 12, 2023

Hi, I think that this PR broke ament-clang-tidy.
As you can see here https://github.com/ament/ament_lint/blob/rolling/ament_clang_tidy/ament_clang_tidy/main.py#L280-L282
ament-clang-tidy ignores directory with an AMENT_IGNORE file when looking for the compilation database.

This PR added the AMENT_IGNORE in the same place where the compilation database ends up being.

@MichaelOrlov
Copy link
Contributor Author

@alsora It looks like the check for the AMENT_IGNORE files is redundant and a bit outdated in ament-clang-tidy

if 'AMENT_IGNORE' in dirnames + filenames:
  dirnames[:] = []
  continue

https://github.com/ament/ament_lint/blob/f10eecd28cdc05746c1f8c0f45984db6052cfd40/ament_clang_tidy/ament_clang_tidy/main.py#L280-L282
The get_compilation_db_files(paths) tries to find compile_commands.json file in the build folder.
But if the source folder has been marked as AMENT_IGNORE the compile_commands.json will not be present in relevant build folder.
It looks like we can remove those AMENT_IGNORE filtering from the ament_clang_tidy

@alsora
Copy link
Contributor

alsora commented Jun 13, 2023

I agree.
It looks like those AMENT_IGNORE checks were added to all linters in this PR ament/ament_lint#81

Most linters look at the source folder, but clang-tidy looks at the build folder to find the compilation database.
This is why only clang-tidy was broken by this change.

I think it's safe to remove the check in clang-tidy.

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

Successfully merging this pull request may close these issues.

7 participants