organicmaps/organicmaps

Fork 21

[cmake] Group targets into folders #5188

Merged

root merged 1 commit from github/fork/AndrewShkrob/cmake/group-targets-into-folders into master

2023-05-25 20:18:15 +00:00

AndrewShkrob commented

2023-05-18 21:08:29 +00:00

Member

Small enhancement for cmake

Moved 3party includes from CMakeLists.txt to 3party/CMakeLists.txt.
Created function omim_add_subdirectory that groups all targets under the subdirectory into one folder (for IDEs that support this feature).

It won't break anything as it only affects the visual representation in the IDE

Before:

Screenshot 2023-05-18 at 21 40 25

After:

Screenshot 2023-05-18 at 22 42 24 Screenshot 2023-05-18 at 22 43 07

Small enhancement for cmake * Moved 3party includes from `CMakeLists.txt` to `3party/CMakeLists.txt`. * Created function `omim_add_subdirectory` that groups all targets under the subdirectory into one folder (for IDEs that support this feature). It won't break anything as it only affects the visual representation in the IDE <details><summary>Before:</summary> <p> <img width="273" alt="Screenshot 2023-05-18 at 21 40 25" src="/uploads/d397d9f9b3a3e34ae2675bd0fc6fa73b/eaf8f9a4-511f-45f1-b5a1-3b8ec94af7e9"> </p> </details> <details><summary>After:</summary> <p> <img width="255" alt="Screenshot 2023-05-18 at 22 42 24" src="/uploads/e18bae28541b3f358cb5238220ccb368/e707b214-a4fe-4fe3-9085-24de1c6dc789"> <img width="256" alt="Screenshot 2023-05-18 at 22 42 41" src="https://github.com/organicmaps/organicmaps/assets/10351358/70f160e3-7c0d-4d7d-b29a-0d8d31c59fa8"> <img width="263" alt="Screenshot 2023-05-18 at 22 42 50" src="https://github.com/organicmaps/organicmaps/assets/10351358/e99d13cb-80ae-42c3-9c28-bff3b324c9ef"> <img width="244" alt="Screenshot 2023-05-18 at 22 43 07" src="https://github.com/organicmaps/organicmaps/assets/10351358/13e7aae3-23a4-4067-8735-34d45d72859a"> <img width="254" alt="Screenshot 2023-05-18 at 22 43 00" src="https://github.com/organicmaps/organicmaps/assets/10351358/0de65835-8a32-404e-b6a8-fa112ae00aba"> </p> </details>

rtsisyk commented

2023-05-19 08:11:34 +00:00

Owner

This looks a bit hacky to achieve visual changes in IDE... What doesn't work if we keep original add_subdirectory() everywhere?

rtsisyk commented

2023-05-19 08:13:50 +00:00

Owner

What doesn't work if we keep add_subdirectory() original?

AndrewShkrob commented

2023-05-19 10:24:39 +00:00

Author

Member

Think of it as a generic solution to group targets recursively. It works even if the project in the subdirectory doesn't support this feature by default.

What doesn't work if we keep original add_subdirectory() everywhere?

Everything will work except that we'll have a mess in the targets list as shown in the "Before" screenshot.
Some libs (especially 3party) produce a lot of targets that may be impossible to determine what library/module they belong to.

F.e. gflags. That's what it looks like after my changes.

Another example is protobuf. We have now a very old version of it (v3, I guess). I'd like to update it to the new one (v23) and link it as a submodule directly. It should fix build warnings when we'll migrate to C++20. But it will produce dozen of targets (with meaningless names) that will mess up our targets list even more. So, that will be a good solution to store all related targets under the 3party/protobuf/... folder.

Think of it as a generic solution to group targets recursively. It works even if the project in the subdirectory doesn't support this feature by default. > What doesn't work if we keep original add_subdirectory() everywhere? Everything will work except that we'll have a mess in the targets list as shown in the "Before" screenshot. Some libs (especially 3party) produce a lot of targets that may be impossible to determine what library/module they belong to. F.e. `gflags`. That's what it looks like after my changes. <img width="240" alt="Screenshot 2023-05-19 at 12 19 02" src="/uploads/97fa2a3be90488d5a623ead5838e570c/f7c57add-697c-4c37-b1c2-602cff47d825"> Another example is `protobuf`. We have now a very old version of it (v3, I guess). I'd like to update it to the new one (v23) and link it as a submodule directly. It should fix build warnings when we'll migrate to C++20. But it will produce dozen of targets (with meaningless names) that will mess up our targets list even more. So, that will be a good solution to store all related targets under the `3party/protobuf/...` folder.

AndrewShkrob commented

2023-05-19 10:24:49 +00:00

Author

Member

Check the answer above

biodranik commented

2023-05-20 20:48:45 +00:00

(Migrated from github.com)

Does it rearrange targets in XCode only? Or also in CLion?
We do not use CMakelists.txt now in XCode. Do you generate XCode project manually?

Does it rearrange targets in XCode only? Or also in CLion? We do not use CMakelists.txt now in XCode. Do you generate XCode project manually?

AndrewShkrob commented

2023-05-21 11:41:24 +00:00

Author

Member

Does it rearrange targets in XCode only?

It depends on the generator used by cmake. Xcode and Visual Studio generators support grouping targets into folders.
But IDE should also support this feature.

Or also in CLion?

CLion supports folder structure for targets. Unfortunately, I wasn't able configure it to make it automatically based on CMakeLists.txt. Perhaps, in future updates they will add support for this feature.

Do you generate XCode project manually?

Yes

> Does it rearrange targets in XCode only? It depends on the generator used by cmake. Xcode and Visual Studio generators support grouping targets into folders. But IDE should also support this feature. >Or also in CLion? CLion supports folder structure for targets. Unfortunately, I wasn't able configure it to make it automatically based on CMakeLists.txt. Perhaps, in future updates they will add support for this feature. > Do you generate XCode project manually? Yes

vng commented

2023-05-21 14:21:39 +00:00

(Migrated from github.com)

I like the idea to take out 3party into separate CMakeList
I don't like _omim_get_all_targets_in_dir in this PR

I don't use XCode for core dev and don't know the problems. But, if you want, use the current solution with xcode/omim.xcworkspace and support it.

We have plans to complex reconsider XCode + cmake. Let's avoid such kind of patches like _omim_get_all_targets_in_dir for now.

- I like the idea to take out 3party into separate CMakeList - I don't like _omim_get_all_targets_in_dir in this PR I don't use XCode for core dev and don't know the problems. But, if you want, use the current solution with xcode/omim.xcworkspace and support it. We have plans to _complex_ reconsider XCode + cmake. Let's avoid such kind of patches like _omim_get_all_targets_in_dir for now.

vng commented

2023-05-21 14:23:07 +00:00

(Migrated from github.com)

So, let's make 3party/CMakeList only. XCode then will group all useless targets inside 3party folder and you even will not see them, right?

So, let's make 3party/CMakeList only. XCode then will group all _useless_ targets inside 3party folder and you even will not see them, right?

vng commented

2023-05-21 14:31:11 +00:00

(Migrated from github.com)

Ok, if XCode still will be ugly, you can put targets workaround code near add_subdirectory(3party) only ..
Or find an option to hide 3party subfolder in XCode (don't know if it's possible).

Ok, if XCode still will be ugly, you can put _targets workaround code_ near add_subdirectory(3party) only .. Or find an option to hide 3party subfolder in XCode (don't know if it's possible).

biodranik commented

2023-05-21 15:26:20 +00:00

(Migrated from github.com)

Is there a way to add upstream protobuf cmake project but import only relevant targets from it?

AndrewShkrob commented

2023-05-21 15:52:04 +00:00

Author

Member

Is there a way to add upstream protobuf cmake project but import only relevant targets from it?

We can disable tests targets but seems nothing more.
One big problem is that it has https://github.com/abseil/abseil-cpp as a 3party which has target base
It's not possible to have two targets with same name - abseil's base and our base
The only solution I figured out is to rename our targets - add the prefix omim_. And we can also add an alias like OMIM::base.

What do you think?

> Is there a way to add upstream protobuf cmake project but import only relevant targets from it? We can disable tests targets but seems nothing more. One big problem is that it has https://github.com/abseil/abseil-cpp as a 3party which has target `base` It's not possible to have two targets with same name - abseil's base and our base The only solution I figured out is to rename our targets - add the prefix `omim_`. And we can also add an alias like `OMIM::base`. What do you think?

vng commented

2023-05-21 16:12:29 +00:00

(Migrated from github.com)

I don't like adding complex code patches because if some 3party libs (or IDEs inconveniences).
Especially renaming our core code base. I suspect that we can easily compile 3party targets into separate 3party folder with literally 5 code lines. Or even don't compile useless targets for us. We need only protobuf lib.

I don't like adding complex code patches because if some 3party libs (or IDEs inconveniences). Especially renaming our core code base. I suspect that we can easily compile 3party targets into separate 3party folder with literally 5 code lines. Or even don't compile useless targets for us. We need only protobuf lib.

biodranik commented

2023-05-21 16:48:40 +00:00

(Migrated from github.com)

Our initial approach was very flexible and lightweight, relatively easy to support (for non header-only 3party libs):

Copy the snapshot of the master or stable release of 3party, by taking only necessary headers/libs.
Wrap them into a convenient project file (now CMakeLists.txt) by copying the required platform flags/settings from the original library for our supported platforms.
Integrate it with our code as a static library.

An alternative solution could be to use protobuf libs from the system, but it may complicate the development setup for contributors.

A complex protobuf submodule can be an issue. Renaming our base classes because of some other target looks badly wrong.

Our initial approach was very flexible and lightweight, relatively easy to support (for non header-only 3party libs): 1. Copy the snapshot of the master or stable release of 3party, by taking only necessary headers/libs. 2. Wrap them into a convenient project file (now CMakeLists.txt) by copying the required platform flags/settings from the original library for our supported platforms. 3. Integrate it with our code as a static library. An alternative solution could be to use protobuf libs from the system, but it may complicate the development setup for contributors. A complex protobuf submodule can be an issue. Renaming our base classes because of some other target looks badly wrong.

AndrewShkrob commented

2023-05-23 21:01:46 +00:00

Author

Member

Only 3party/CMakeLists.txt left under this PR

vng commented

2023-05-25 17:51:22 +00:00

(Migrated from github.com)

approved this merge request

biodranik commented

2023-05-25 20:18:10 +00:00

(Migrated from github.com)