search

corrosion-rs / corrosion

Marrying Rust and CMake - Easy Rust and C/C++ Integration!
https://corrosion-rs.github.io/corrosion/
MIT License
1.02k stars 97 forks source link

Copy pdb to runtime/library output dir as fallback #486

Closed Nopey closed 5 months ago

Nopey commented 5 months ago

Motivation

When setting just RUNTIME_OUTPUT_DIRECTORY (or LIBRARY_OUTPUT_DIRECTORY), normal cmake targets will direct MSVC to output the EXE/DLL to that directory; MSVC will then infer that the PDB belongs there too. As Corrosion uses a copying-based approach, it currently does not have this behavior.

Repro

include(FetchContent)
FetchContent_Declare(Corrosion GIT_REPOSITORY https://github.com/corrosion-rs/corrosion.git GIT_TAG v0.4)
FetchContent_MakeAvailable(Corrosion)

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "my_runtime_dir")

# This CXX target's PDB will be put in the CMAKE_RUNTIME_OUTPUT_DIRECTORY
add_executable(cpp_helloworld main.cpp)

# This Rust target's PDB will NOT be put in the CMAKE_RUNTIME_OUTPUT_DIRECTORY
corrosion_import_crate(MANIFEST_PATH rust-helloworld/Cargo.toml)

How/What

This PR adds fallback logic that triggers when PDB_OUTPUT_DIRECTORY is not set on a corrosion target-- if the target is an executable, it uses the RUNTIME_OUTPUT_DIRECTORY instead (and LIBRARY_OUTPUT for shared libraries).

Nopey commented 5 months ago

Got scared when my build broke after rebasing from 0.4.7 to master-- was just 9f5833b4a4802ca70175a0f740ef645223fb4961 breaking my (genexp-in-outputdir-abusing) build 😅

jschwe commented 5 months ago

Thanks for PR. It sounds reasonable - I'll have a more detailed look at the changes later.

jschwe commented 5 months ago

I tried pushing a test but it is failing :(. It's quite likely that the issue is in the test I added, but I guess I'll need to test the change on my Windows machine. Might take another couple of days until I have time to investigate.

Nopey commented 5 months ago

Thanks for adding tests!

I broke the implementation of this feature when I tried to make it support executables (after getting my own build working, that only needed it to work on DLLs)

Shouldn't have been checking the TYPE property of the target-- it's always INTERFACE_LIBRARY. Instead, the caller of _corrosion_copy_byproduct can now specify multiple properties to check for the destination.