CMake: Set plugins runtime output directory even when the rest are not set

[hugoam]

Hello ! Our project only sets CMAKE_RUNTIME_OUTPUT_DIRECTORY but not the other two CMake variables, because those tend to mess with the way Visual Studio produces and consumes .lib so we don't want to open that can of worms.

This wasn't an issue before as we only consumed Magnum in a prebuilt way, but now I'm including magnum as part of our CMake tree for development convenience, and I'd still like the .dll to end up in the correct plugin subfolder without having to do some ugly stuff.

So this change proposes to set the RUNTIME_OUTPUT_DIRECTORY of the plugins when only CMAKE_RUNTIME_OUTPUT_DIRECTORY is set even if the other two variables are not set. In order to facilitate the change, I also factored the directory selection logic of the plugins to a single place since it was relatively complex code that was copied in more than 20 locations.

Let me know what you think and if you see issues with this approach, thanks :)

[mosra]

I saw you forking the repos so I went and saw this change already. Especially the massive deletion I like :D

Before I go any further, is #486 something you could fix as part of this PR as well? Should be hopefully easier now that it's not duplicated all over the place.

[mosra]

Oh, and apart from that, it seems that CircleCI is not running the PR :/ Not sure what's exactly to blame, for some PRs it works for some not. Assuming you have CircleCI connected with your github account, maybe you need to set up building of your forked repo in your CircleCI profile? Or the inverse, tell it to stop building the fork? Not sure.

Maybe @pezcode could help here, for him it worked always... somehow.

[hugoam]

Before I go any further, is #486 something you could fix as part of this PR as well? Should be hopefully easier now that it's not duplicated all over the place.

I'm thinking about it, but I'm unsure what would be the proper way to fix it. One way to see the problem is that it actually might be Conan's responsibility to set the CMAKE_RUNTIME_OUTPUT_DIRECTORY variable to a generator expression that discards the configuration, instead of setting CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG and CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE directly

One thing is that if we want to handle those configs variables explicitly we at least have to handle Debug, Release, RelWithDebInfo and MinSizeRel, and maybe more 😅

[hugoam]

I added a commit which shows how handling each output directory and each config would look like, but we can always remove it if we deem it's not worth it 😄

[pezcode]

Maybe @pezcode could help here, for him it worked always... somehow.

I don't recall ever having to change a CircleCI configuration somewhere, but I do have an account there that's linked to my GitHub account if that makes any difference

[hugoam]

I did just enable CircleCI on my own repo, will see if that changes anything

[hugoam]

Hmmm, CircleCI works now. I think what made it work is that Allow Uncertified Orbs was not checked in my CircleCI account settings, and Magnum needs this to be enabled to run the codecov thing. This is pretty silly though especially that there is no way to know that this is the cause. I just discovered it because I tried to enable CircleCI on my own Magnum fork, and the job failed because of this setting

[hugoam]

Incorporated your change and in the magnum-plugins PR too Most likely once this is merged we can close https://github.com/mosra/magnum/issues/486 too 😄

[mosra]

Thanks! Yup, #486 would be solved by this.

One last last thing, just to make sure -- I see a Fix Build NO_DEPRECATED commit here, it disappeared before but now it's back. Is it still needed or are the changes I made in f049ba9012bd2f9291cba0cb01238f73c272941c enough to make the non-deprecated build work on MSVC? If it's not needed anymore, I'll skip it during merge.

[hugoam]

Oh, yeah, sorry for that, it's a commit I have locally because I build with the deprecated setting off for some reason and so I need it for the build to succeed. Yes I think some changes are needed because those are from the recent Manager.hpp header creation, but we can see about that separately, I removed the unrelated commit 😅

[mosra]

Ah so f049ba9012bd2f9291cba0cb01238f73c272941c didn't fix everything? If you get a chance, post the full error here and I'll attempt further. Oh, and 45c1a7091f6386daf2b1075c9fb05d508d94f2b4 should fix the Manager.hpp issue. (I really have to create a CI build for this configuration, it's getting ridiculous, sorry.)

Will merge this in a moment, together with the plugins PR.

[mosra]

Merged as bcb96eba36c02afedaf860c26c205985a10226bc, thank you!

And 45c1a7091f6386daf2b1075c9fb05d508d94f2b4 is in master now as well, so let me know what's left to make non-deprecated builds finally work :)