EXCLUDE_FROM_ALL goes into the FetchContent_Declare

[tizianoGuadagnino]

This tries to fix the FetchContent population error that we introduced many months before. We were checking for a depname_Populated without using `FetchContent_GetProperties(depname)'. This resulted in cmake error as it was trying to populate the same dependency multiple times if KISS was "fetch contented" by another cmake project that was also "fetch conteting" one of the KISS dependencies. WIP

[tizianoGuadagnino]

This KISSfy (more) the build system on the FetchContent side. Ready for your feedbacks guys @nachovizzo @benemer.

[tizianoGuadagnino]

Proposal to @nachovizzo: we can merge the three PRs (This one, #327, and benedikt/remove_personal_repos) about the build system in this one and have all the related changes in one place.

[nachovizzo]

Guys I'm a bit lost with these 3 PR's

In any case, EXCLUDE_FROM_ALL in the Fetchcontent was only introduced in CMake 3.28, which literally renders all these PRs wrong AFIK

[tizianoGuadagnino]

I give you an order of creation:

1. Rename targets, which is where we started Change target names #327.
2. This triggers thoughts about some dependencies being populated by multiple projects (if you build KISS and MapClosures for example), which in our current state implies that the build system core dumps. We should somehow handle the case in which Eigen as been already populated which, with FetchContent_MakeAvailable does already.
3. We want to make the dependencies cmake scripts cleaner (with no personal repos) just because Remove personal workspaces #329

Thats the situation.

[nachovizzo]

Thanks @tizianoGuadagnino. But were you guys aware of the cmake increase on the version???

[tizianoGuadagnino]

Yes, I just wanted to be sure that it is fixable in this way and discuss it. Increasing the CMake version is a mess, and I know, but we need to find a solution to this; otherwise, people might have trouble building on top of our stuff.

There is a very ugly if else logic that can replace the populate checks that MakeAvailable so that we don't need cmake > 3.22. I just though it is worth a discussion here

[nachovizzo]

Yes, I just wanted to be sure that it is fixable in this way and discuss it. Increasing the CMake version is a mess, and I know, but we need to find a solution to this; otherwise, people might have trouble building on top of our stuff.

There is a very ugly if else logic that can replace the populate checks that MakeAvailable so that we don't need cmake > 3.22. I just though it is worth a discussion here

Ok agree, fixing the problem is fine, but I'm not in favor of bumping the version at all. I understand that this will beautify the build system, and I tried this in the past as well and always dream about that. But we should in best cases expect most users will have cmake 3.22.

One option would be to add this nice addition with 3.28 and guard it with an if/else, similar to how we do now

[tizianoGuadagnino]

Mmm, I didn't think about this one. Let me have a look, then ;)

[nachovizzo]

So, let me wrap up this tsunami:

The only neccessary change is #327, then #328 and #329 are just proposals on "nice to have" . Given the versioning I'd say this PR should be closed

I'm trying to understand what really needs fixing ;)

[tizianoGuadagnino]

I will say #328 is not just nice to have but necessary if someone needs to use KISS as a library.

At times, @benemer and I have received 5 PRs as tsunami, so I think that is fine. However, we should not close this without all agreeing that those are marginal changes (which we don't think they are).

Fix for the versioning in this PR with the guarding that we just discuss it is coming anyways.

[nachovizzo]

I will say #328 is not just nice to have but necessary if someone needs to use KISS as a library.

I will say #328 is not just nice to have but necessary if someone needs to use KISS as a library.

At times, @benemer and I have received 5 PRs as tsunami, so I think that is fine. However, we should not close this without all agreeing that those are marginal changes (which we don't think they are).

Fix for the versioning in this PR with the guarding that we just discuss it is coming anyways.

Ok I missed this:

We were checking for a depname_Populated without using `FetchContent_GetProperties(depname)'.

I got confused sorry.

But why don't just fixing that ?

[nachovizzo]

amashing

[tizianoGuadagnino]

Fratis, sorry for all the confusion, but this simple change actually changes a lot in terms of the community's ability to extend the project to more cool robotics stuff, which is what we mostly care about.

I verify the change on my dummy project and it works.

[benemer]

Awesome!

🏄 🏄 🏄