Better integration with FetchContent/ExternalProject/CPM/...

palacaze / sigslot

A simple C++14 signal-slots implementation

MIT License

709 stars 97 forks source link

Better integration with FetchContent/ExternalProject/CPM/... #24

Closed OlivierLDff closed 3 years ago

OlivierLDff commented 3 years ago

Hi, I'm using sigslot with FetchContent. So:

I don't need install
tests & examples should be disabled by default
tests & examples targets should be namespace to avoid conflict with any other project, if I need to enable examples/tests somehow

I have that in my dev branch : https://github.com/OlivierLDff/sigslot/tree/dev

No original behavior got changed if you are only using this repo to install, except the namespacing of examples/tests targets. Tell me if you would be interested in such a PR, if it should be divided, etc...

I tried to separate each features into a commit.

Edit : I also added CI with github actions if interested.

palacaze commented 3 years ago

Those are some nice changes, I will gladly merge them! Send me a PR when ready.

A few comments beforehand:

The test target is currently configured to launch CTest on build, so your "Unit Tests" step is not needed
The nproc command is part of coreutils on GNU/Linux, I do not think Windows shell supports it. An alternative may be to use the Ninja generator

OlivierLDff commented 3 years ago

Yes you are right I see that now, I will remove the unit test step.
Is nproc a problem? Build run fine.

Do you want separate PR for ci/cmake, or one is ok?

palacaze commented 3 years ago

It builds but this may be a fluke. Somehow powershell does not trip over the $(nproc) expression. As per cmake's documentation, using cmake --build -j without specifying the number of processes will default to what the underlying make tool uses. I think it would be safer to just remove -j on MS builds.

One PR is fine with me.