Windows build: Suggestion for fixing a build issue with ninja generator

[irieger]

Using the ninja generator via CMake, as conan does for Windows builds, link & manifest addition are handled differently - aka. in separate steps - compared to Visual Studio generator. It seems that the VS generator adds the manifest parameter to the link command which passes it down - thus add_link_options - but the ninja generator creates a separate direct call to mt.exe which needs the manifest passed.

So with just passing the generated file as a source file, CMake supposedly should handle both generators accordingly.

Disclaimer: This change & commit message was manually done but prepared with input from Claude Opus 4.7 as I don't run Windows nor have much experience with the build specifics on Windows.

So I hope this does indeed still work with other Windows builds directly running Visual Studio. As I said above, I don't run crap OS so can't check it myself, but at least the same patch applied to 1.7 makes the conan build work.

So here is the build error which triggered me to attempt the fix: https://c3i.jfrog.io/artifactory/cci-build-logs/cci/prod/PR-30166/2/package_build_logs/build_log_bmx_1_7_e12f988ab67c48ebbe2d9a197416d038_c81ab53d7aec4674643bcc7bed3abc5dbe54a020.txt
And now with the patch, all green: https://c3i.jfrog.io/artifactory/cci-build-logs/cci/prod/PR-30166/4/package_build_logs/build_log_bmx_1_7_cb8f9f807e7dd4fb12b61105a45592a2_9c0898c88625b265470432527a881c29db1cec84.success.txt

Conan PR with that patch: conan-io/conan-center-index#30166

[scenarnick]

Thanks for your submission. I'm working on macOS and Linux (ARM64) myself so I cannot check directly. Will see how to test.

[haraldjordan]

I'll need to look into this because its a consequence of the long path windows feature. The Patch is doing something i tried to avoid, e.g. consider the manifest in each individual project instead of centralized. If it turns out that this is the only solution, we might want to check if the /MANIFEST:EMBED in apps/CmakeLists is still needed

[irieger]

I'll need to look into this because its a consequence of the long path windows feature. The Patch is doing something i tried to avoid, e.g. consider the manifest in each individual project instead of centralized. If it turns out that this is the only solution, we might want to check if the /MANIFEST:EMBED in apps/CmakeLists is still needed

Ok, but would you generally be good with the patch doing that for now so that the 1.7 can land in Conan, and we try to find a better solution in the meantime until the next release?

[haraldjordan]

Not sure about it yet, i would not want to pull changes just to revert them short time later. Fortunately we have some experience with all the building now so i hope my investigations go very fast now :)

[irieger]

Think that was a misunderstanding. I meant if you see any problem with the code to be part of the packaging for conan (process in conan is to pull the source package - and if needed - apply small patches like this one)?

Unless you plan to fix it soon and then directly release it, it would be nice if we can agree on that to have the conan stuff merged. Also the impact I'd say should be relatively small, as the main output is the libraries which aren't influenced anyway.

[haraldjordan]

Understood, so no as long as the build process works and the manifest is there, i don't see a problem with it.