Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmake: Use cargo --target-dir #10346

Closed
wants to merge 1 commit into from
Closed

cmake: Use cargo --target-dir #10346

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions CMakeLists.txt
View file
Open in desktop
Expand Up @@ -59,12 +59,13 @@ FILE(GLOB sources src/* src/*/* src/*/*/*)
# Define a function to link dependencies.
function(FISH_LINK_DEPS_AND_SIGN target)
add_custom_command(
OUTPUT ${rust_target_dir}/${rust_profile}/${target}
OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${Rust_CARGO_TARGET}/${rust_profile}/${target}
COMMAND ${CMAKE_COMMAND} -E env
${VARS_FOR_CARGO}
${Rust_CARGO} ARGS build --bin ${target}
$<$<CONFIG:Release,RelWithDebInfo>:--release>
--target ${Rust_CARGO_TARGET}
--target-dir ${CMAKE_CURRENT_BINARY_DIR}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already override the target dir witih CARGO_TARGET_DIR which is passed via VARS_FOR_CARGO.
This is why docker/docker_run_tests.sh docker/opensuse-tumbleweed.docker works, which mounts the source tree read-only.
But it looks like our "CARGO_TARGET_DIR=${FISH_RUST_BUILD_DIR}" is inconsistent with ${rust_target_dir}.
We should probably fix that to use the same target dir everywhere, at least inside cmake land.

I think today the top-level target/ directory should only be created by cargo b and things like rust-analyzer and cargo clippy.
I don't know if there's a good way to override this but long-term we should try to get rid of cmake anyway.
Right now, adding a new source file requires a manual cmake rerun.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already override the target dir witih CARGO_TARGET_DIR which is passed via VARS_FOR_CARGO.

Huh, cool. This is unnecessary then.

I think my "target" might have been created by rust-analyzer then.

But it looks like our "CARGO_TARGET_DIR=${FISH_RUST_BUILD_DIR}" is inconsistent with ${rust_target_dir}.

"rust_target_dir" is FISH_RUST_BUILD_DIR plus the arch-os-triple.

we should try to get rid of cmake anyway

Of course.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make a .cargo/config.toml with target-dir = "build/cargo/build", which rust-analyzer and friends pick up

https://doc.rust-lang.org/cargo/reference/config.html#configuration

${CARGO_FLAGS}
${FEATURES_ARG}
DEPENDS ${sources} src/bin/${target}.rs
Expand All @@ -75,9 +76,9 @@ function(FISH_LINK_DEPS_AND_SIGN target)
)
add_custom_target(${target} ALL
COMMAND "${CMAKE_COMMAND}" -E copy
"${rust_target_dir}/${rust_profile}/${target}"
${CMAKE_CURRENT_BINARY_DIR}/${Rust_CARGO_TARGET}/${rust_profile}/${target}
"${CMAKE_CURRENT_BINARY_DIR}"
DEPENDS ${rust_target_dir}/${rust_profile}/${target}
DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/${Rust_CARGO_TARGET}/${rust_profile}/${target}
)
codesign_on_mac(${target})
endfunction(FISH_LINK_DEPS_AND_SIGN)
Expand Down