Fix support for hostbuild executables when cross-compiling to non-windows on windows hosts #437

tronical · 2023-09-08T14:07:50Z

Defer the determination of whether to apply a .exe suffix for the byproduct copying and imported location setting if possible
Determine the .exe suffix first based on the existence of the hostbuild property and then fall back Rust_CARGO_TARGET_OS

tronical · 2023-09-08T14:11:50Z

This is an initial attempt at fixing the issue where we cross-compile on Windows to a non-Windows target but still build and call a host-build tool.

I don't quite know how to best automate testing this, but this is what I did manually:

Install ESP-IDF https://docs.espressif.com/projects/esp-idf/en/stable/esp32/get-started/windows-setup.html#standard-setup-of-toolchain-for-windows , a toolchain/framework for cross-compiling for Espressif MCUs.
Within an ESP-IDF enabled command prompt, configure Corrosion with tests enabled and -DRust_CARGO_TARGET=tensa-esp32-none-elf
Run ctest -C Debug -R hostbuild . to get build coverage for the described scenario. The test fails to build at the byproduct copying stage before the patch, and succeeds afterwards. The imported location path I verified via the cmake debug output Corrosion produces.

tronical · 2023-09-08T14:26:25Z

I've now been able to verify that the imported location bit also works. The "higher level" test case is to clone Slint from git, and from an ESP-IDF prompt change into examples/carousel/esp-idf/s3-box and run idf.py build. It fails before this patch trying to copy slint-compiler as byproduct, when it should be slint-compiler.exe. After this patch the build passes.

tronical · 2023-09-08T14:27:44Z

According to the test failures in the CI here, it seems that this requires further work.

tronical · 2023-09-08T14:29:50Z

Ahh, just if(${target_type} STREQUAL "EXECUTABLE") is not enough to verify that the suffix handling should only be applied to executables, as the byproduct in question might be the .pdb for the .exe.

tronical · 2023-09-08T14:53:40Z

cmake/Corrosion.cmake

@@ -321,7 +358,7 @@ function(_corrosion_copy_byproduct_deferred target_name output_dir_prop_name car
    )
 endfunction()

-function(_corrosion_call_copy_byproduct_deferred target_name output_dir_prop_name cargo_build_dir file_names)
+function(_corrosion_call_copy_byproduct_deferred target_name output_dir_prop_name cargo_build_dir file_names is_binary)


The extra parameter is rather ugly. I'm wondering if it's worth porting this over to cmake_parse_arguments. But I guess latest in this function it would all have to be unpacked and packed again, so maybe not worth it?

hmm, maybe we could just remove all named parameters to this function and pass [[${ARGN}]] to the deferred function.

Good idea, I’ll try that.

tronical · 2023-09-08T14:55:32Z

generator/src/subcommands/gen_cmake/target.rs

                )
            endif()
            if(pdb_byproduct)
                _corrosion_copy_byproducts(
-                    {target_name} PDB_OUTPUT_DIRECTORY \"${{cargo_build_out_dir}}\" \"${{pdb_byproduct}}\"
+                    {target_name} PDB_OUTPUT_DIRECTORY \"${{cargo_build_out_dir}}\" \"${{pdb_byproduct}}\" FALSE
                )
            endif()
            if(bin_byproduct)
                _corrosion_copy_byproducts(


I reckon this will generally go wrong, as there's no suffix handling on the caller side here. What's the status of this code path?

I was planning to remove the legacy generator for v0.5 and require a newer Cmake version (was thinking of 3.22 as in ubuntu 22.04). I could do it now.

I’m curious: is the newer cmake version requirement related to the removal of the generator?

The legacy Generator enabled CMake support for CMake < 3.19 (3.19 introduced JSON parsing support). There was also some Multi-Config related code which was conditionally enabled based on CMake 3.20 / 3.21, so I decided to declare CMake 3.22 as a minimum supported version to have a bit of wiggle-room, while still supporting ubuntu-22.04 out of the box.

Thanks, makes sense to me. Looking forward to the update :)

tronical · 2023-09-08T15:00:41Z

Regarding testing, perhaps this is reproducible to some extent in the CI by just doing say -DRust_CARGO_TARGET=aarch64-unknown-linux-gnu (after installing such toolchain on Windows). I'm not sure entirely how to configure test.yaml for this.

tronical · 2023-09-08T15:06:13Z

Yep, this is much easier to reproduce on Windows without ESP-IDF by simply adding rustup target add aarch64-unknown-linux and then configuring with -DRust_CARGO_TARGET=aarch64-unknown-linux-gnu.

tronical · 2023-09-08T15:08:41Z

I'm going to need guidance though how to extend test.yaml best, if it's required to cover this in the CI.

jschwe · 2023-09-11T06:56:08Z

Currently most of the CI logic sits in this action.yaml. But I would prefer to remove most of the logic from there and instead use CMake toolchain files.

Currently, the action has the assumption that host_os == target_os (and that the OS is either windows, mac or linux), so at the very least that would need to be changed. But it would probably be simpler to do the refactoring towards toolchain files instead.

if it's required to cover this in the CI.

I would prefer if this would be tested, otherwise it's hard to ensure that it doesn't break again.

jschwe · 2023-09-12T06:14:47Z

Short update: I've experimented a bit with toolchain files + CMakePresets.json and it seems quite promising.

jschwe · 2023-11-02T09:23:02Z

Sorry for the delay, this MR fell a bit off my radar.

I'm going to need guidance though how to extend test.yaml best, if it's required to cover this in the CI.

Do you know what dependencies you need to install for the test and what parameters CMake needs to be configured with?
Once we have that, adding a test should be very straight forward.

tronical · 2023-11-02T16:33:41Z

Yes, I think it’s sufficient to add rustup target add aarch64-unknown-linux and just configure with -DRust_CARGO_TARGET=aarch64-unknown-linux-gnu to reproduce the failure and see the tests pass with this PR.

jschwe · 2023-11-02T16:37:46Z

What about installing a cross c compiler for the linking? I think the Linux targets all still default to using cc as the linker and not lld.

tronical · 2023-11-03T10:36:14Z

I think for all tests you'd indeed need a cross c compiler for linking. But for testing this particular issue I've just used this:

Configure with -DRust_CARGO_TARGET=aarch64-unknown-linux-gnu
Run ctest -C Debug -R hostbuild .

With master it fails:

Test project C:/dev/corrosion/build
    Start 37: hostbuild_build
1/3 Test #37: hostbuild_build ...................***Failed   14.96 sec
    Start 38: hostbuild_run_rust-host-program
2/3 Test #38: hostbuild_run_rust-host-program ...***Not Run (Disabled)   0.00 sec
    Start 39: hostbuild_cleanup
3/3 Test #39: hostbuild_cleanup .................   Passed    0.15 sec

50% tests passed, 1 tests failed out of 2

Total Test time (real) =  15.21 sec

The following tests did not run:
         38 - hostbuild_run_rust-host-program (Disabled)

The following tests FAILED:
         37 - hostbuild_build (Failed)
Errors while running CTest
Output from these tests are in: C:/dev/corrosion/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.

...
  Error copying file (if different) from "C:/dev/corrosion/build/test/hostbuild/build-hostbuild/x64/Debug/cargo/build/x86_64-pc-windows-msvc/debug/rust-host-program" to "C:/dev/corrosion/build/test/hostbuild/build-hostbuild".
...

But this PR it succeeds.

…dows on windows hosts - Defer the determination of whether to apply a .exe suffix for the byproduct copying and imported location setting if possible - Determine the .exe suffix first based on the existence of the hostbuild property and then fall back Rust_CARGO_TARGET_OS

…actual bin byproduct and not .pdb files

tronical · 2023-11-03T10:38:44Z

Rebase pushed.

jschwe · 2023-11-03T18:32:44Z

Thanks!

ogoffart · 2023-11-28T16:14:16Z

Any chances in backporting that in the 4.x branch and having a release?
This issue is preventing our Windows users to use Slint for MCUs from C++.

* Fix support for hostbuild executables when cross-compiling to non-windows on windows hosts - Defer the determination of whether to apply a .exe suffix for the byproduct copying and imported location setting if possible - Determine the .exe suffix first based on the existence of the hostbuild property and then fall back Rust_CARGO_TARGET_OS Cherry-picked from b8a6b26 * Attempt to limit deferred byproduct .exe suffix handling only to the actual bin byproduct and not .pdb files Cherry-picked from 85b8822 * Update RELEASES.md for v0.4.5 --------- Co-authored-by: Simon Hausmann <[email protected]>

tronical marked this pull request as ready for review September 8, 2023 14:24

tronical commented Sep 8, 2023

View reviewed changes

tronical added 2 commits November 3, 2023 11:36

Attempt to limit deferred byproduct .exe suffix handling only to the …

85b8822

…actual bin byproduct and not .pdb files

jschwe self-requested a review November 3, 2023 17:14

jschwe approved these changes Nov 3, 2023

View reviewed changes

jschwe merged commit 85b8822 into corrosion-rs:master Nov 3, 2023
35 checks passed

jschwe mentioned this pull request Nov 28, 2023

Backport #437 windows hostbuild fix to v0.4 #462

Merged

This was referenced Jan 16, 2024

Error cross-compiling with C++ from Linux to Windows slint-ui/slint#2508

Closed

[Bug]: cross compilation from linux to windows with build tools fails. #476

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix support for hostbuild executables when cross-compiling to non-windows on windows hosts #437

Fix support for hostbuild executables when cross-compiling to non-windows on windows hosts #437

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

tronical Sep 8, 2023

jschwe Sep 8, 2023

tronical Sep 12, 2023

tronical Sep 8, 2023

jschwe Sep 8, 2023

tronical Sep 12, 2023

jschwe Sep 12, 2023

tronical Sep 12, 2023

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

jschwe commented Sep 11, 2023

jschwe commented Sep 12, 2023

jschwe commented Nov 2, 2023 •

edited

Loading

tronical commented Nov 2, 2023

jschwe commented Nov 2, 2023

tronical commented Nov 3, 2023

tronical commented Nov 3, 2023

jschwe commented Nov 3, 2023

ogoffart commented Nov 28, 2023

Fix support for hostbuild executables when cross-compiling to non-windows on windows hosts #437

Fix support for hostbuild executables when cross-compiling to non-windows on windows hosts #437

Conversation

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

tronical commented Sep 8, 2023

jschwe commented Sep 11, 2023

jschwe commented Sep 12, 2023

jschwe commented Nov 2, 2023 • edited Loading

tronical commented Nov 2, 2023

jschwe commented Nov 2, 2023

tronical commented Nov 3, 2023

tronical commented Nov 3, 2023

jschwe commented Nov 3, 2023

ogoffart commented Nov 28, 2023

jschwe commented Nov 2, 2023 •

edited

Loading