Fix output_directory support when not used as target property

[tronical]

In CMake, for example RUNTIME_OUTPUT_DIRECTORY is a target property. It is documented to be initialized from CMAKE_RUNTIME_OUTPUT_DIRECTORY, so it's not unusual for CMake projects to just set the latter variable in order to place all affected targets into the directory, instead of setting a per-target property on each manually.

Since Corrosion respects the target property, it makes sense to assume that just setting the CMAKE_ variable would also work. Oddly, the get_property returns -NOTFOUND when only the CMAKE_ variable is set and not the target property. To "work around" that, this patch implements the fallback manually.

[tronical]

I need some advice on how to best add test coverage for this. test/output directory/output directory/CMakeLists.txt tests the target properties, but I'd rather not duplicate the whole test. Should I duplicate the series in test/output directory/CMakeLists.txt perhaps and add an option for test/output directory/output directory/CMakeLists.txt to select whether to set the target property or the global variable?

[jschwe]

it makes sense to assume that just setting the CMAKE_ variable would also work. Oddly, the get_property returns -NOTFOUND when only the CMAKE_ variable is set and not the target property.

I guess CMake doesn't set this property on IMPORTED or INTERFACE targets. I'm not sure why I didn't notice this earlier. Thanks for reporting this!

Instead of complicating the checking part like this PR proposes, I would prefer if Corrosion would initialize the properties as CMake would do.

Since respecting OUTPUT_DIRECTORY anyway requires CMake 3.19, I would suggest editing _generator_add_cargo_targets() in CorrosionGenerator.cmake and add a loop at the end of the function that iterates over created_targets and sets the 5 properties if the relevant CMAKE_ variable is defined. Should be very simple to do.

This approach has the advantage of working as expected if (for some reason) a user has two or more corrosion_import_crate calls in the same CMakeLists.txt file and changes the CMAKE_ values in between the calls.

Should I duplicate the series in test/output directory/CMakeLists.txt perhaps

Yes, please

and add an option for test/output directory/output directory/CMakeLists.txt to select whether to set the target property or the global variable?

Thats one option. We could also just duplicate the rust package (and have a CMake test project with 2 corrosion_import_crate calls). In my opinion, less logic in output directory/output directory is preferable.

[tronical]

Thank you for the explanation. The observed behavior makes sense now and I agree about the way of fixing it. I'll look into that.

[tronical]

I've implemented this now, and it seems to work to some extent. Local on macOS with ninja, I'm having the issue that mysteriously CMAKE_ARCHIVE_OUTPUT_DIRECTORY isn't respected for the static lib test.

[tronical]

Lol, shortly after posting the comment I found the issue. So it's passing locally now.

[tronical]

I guess the remaining failures are due to lack of support for CMAKE_CONFIGURATION_TYPES. I'll look into that tomorrow.

[tronical]

Ok, at least the CI is green now :-).

[jschwe]

Looks good now. Thanks for fixing this!
You mentioned you wanted to squash, so I'll wait for that before merging.

[tronical]

Done :)

[tronical]

Hm, for some reason GitHub isn't show that force push.