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.

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

Fix windows build. #3222

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

redpolline
Copy link
Contributor

Should fix #3092.

@github-actions github-actions bot added the build Related to the build process label Feb 22, 2024
@redpolline redpolline force-pushed the fix_windows_build branch 2 times, most recently from 124e676 to e8f1258 Compare February 22, 2024 20:08
@redpolline
Copy link
Contributor Author

Well that's odd, the Windows CI build doesn't seem to set CMAKE_SYSTEM_NAME to Windows. Maybe try with VCPKG_TARGET_TRIPLET....

@redpolline
Copy link
Contributor Author

Well that last one was a build system setup error: [NuGet] Response status code does not indicate success: 500 (Internal Server Error).

@redpolline redpolline force-pushed the fix_windows_build branch 3 times, most recently from d73122c to 0866622 Compare February 22, 2024 22:40
@redpolline
Copy link
Contributor Author

OK, The CI is setting CMAKE_SYSTEM_NAME to Windows. So why is it claiming that CMP0144 isn't being set?

@redpolline
Copy link
Contributor Author

OK, defining the CMP default on the command line works. (I guess CMP0069 isn't working here either then....) So that's one problem down.

Now just I need to figure out why the CI build is rejecting all of the boost libraries. So let's try setting Boost_DEBUG and Boost_VERBOSE.

@redpolline
Copy link
Contributor Author

OK, it's seemingly due to the Boost_COMPILER being wrong. (vc140 instead of vc143.) Let's try to override it manually and see if it works.

@redpolline redpolline force-pushed the fix_windows_build branch 11 times, most recently from b7cd98c to 82968be Compare February 23, 2024 08:17
@redpolline
Copy link
Contributor Author

For best results just have the CI use the vcpkg installed boost.

@Macdu
Copy link
Contributor

Macdu commented Feb 23, 2024

Please do not modify the CI files, there is no reason to do so as it works perfectly fine right now.

# As the libraries are not fully ABI-compatible with each other.)
#
# TL;DR: Fix Boost by not calling find_package(Boost ...) if it's already been found once.
if (NOT VITA3K_Boost_FOUND)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (NOT VITA3K_Boost_FOUND)
if (NOT TARGET Boost::boost)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I've stated in the long post below, this won't work.

Boost already tries to do this itself in it's boost_*-config.cmake files as the very first functional statement, and again in the various variant files (for me at line 74 in both). In all cases Boost's own attempts fail.

Sadly, that means we have to keep track of this ourselves.

# instead adding the found definitions to the project's link dependencies directly. With no way to perform library detection only.
# Worse, get_boost() and boost_compile() runs during configure so we don't have a build configuration yet, and therefore do not know what
# build of Boost (Release or Debug) to use at that point. This means that calling find_package(Boost ...) more than once can result
# in a project that has both the Debug and Release libraries added as dependencies. (Which will cause a link failure under MSVC.
Copy link
Contributor

Choose a reason for hiding this comment

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

How can boost try to use both a Debug and Release build? It will by default use a build matching the current target build type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When cmake is initially called with the preset option, the build configuration isn't set by VS2022. As such everything winds up defaulting to the Debug build as that's the first entry in cmake's default configuration list. This causes Boost to add it's debug dependencies to the build when get_boost() calls find_package().

Later, when cmake is invoked again with the build option, the config option is also given. This sets the build configuration to a specific type. (Whatever the user selected in VS2022's GUI.) As long as that configuration was "Debug" the build will succeed. As get_boost()'s call to find_package() will effectively be a no-op.

If the user has selected a different build configuration however, then the repeated call to find_package() will append a new Boost library and set of include headers to the target. VS2022 can not handle this, and thus we get #3092.

Re-running the build, without changing the build configuration in VS2022's GUI again, however will get rid of the old incorrect dependency and allow the build to succeed, but that's inconvenient to those trying to follow the manual build instructions for Windows. We also cannot remove the incorrect dependency ourselves once it's added, as cmake doesn't provide a reliable option / command to do so. As such our only option is to prevent the second call to find_package() from being made in the first place.

As to my theory as to why this behavior happens and why the work around, well, works:

In my build of Vita3K's external boost::filesystem we have these files at Vita3K\build\windows-vs2022\external\boost\lib\cmake\boost_filesystem-1.82.0:
boost_filesystem-config.cmake
boost_filesystem-config-version.cmake
libboost_filesystem-variant-vc143-mt-gd-x64-1_82-static.cmake
libboost_filesystem-variant-vc143-mt-x64-1_82-static.cmake

The actual call to add_library() / set_target_properties() is in the variant files. (in both variants at line 75 & 77 for me.) With the config files effectively re-implementing cmake's generator expressions via the various flags: Boost_USE_SINGLE_VARIANT / Boost_USE_RELEASE_LIBS / Boost_USE_DEBUG_LIBS / etc. (I guess this predates cmake's generator expressions? But there is limited use of them....)

There is code in there to block calling add_library() / set_target_properties() using the same suggested change you made above, i.e. if(NOT TARGET Boost::filesystem), but it fails here. My guess is that cmake isn't keeping all of the information between cmake invocations because of the changed build configuration type, but cmake is reusing the previous list of libraries and includes for each project as a template. As such, if (NOT TARGET ...) fails as cmake needs to reconfigure it based on the new build configuration.

Which makes sense. That's the entire point of cmake's generator expressions. Cmake's preset stage is used to generate the default dependency list. Then cmake's build stage, with it's defined build configuration type, is only supposed to append what is specifically needed for that specific build configuration. I.e. Cmake's build stage is not supposed to redefine core dependencies common to all build configurations.

CMakeLists.txt Outdated
Comment on lines 146 to 147
set(Boost_USE_RELEASE_LIBS $<CONFIG:Release>)
set(Boost_USE_DEBUG_LIBS $<CONFIG:Debug,RelWithDebugInfo>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug builds are meant to be used for ... debug builds.
As its name implies RelWithDebugInfo is a release build (with debug info), only use release builds dependencies with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, turns out that's actually not required to fix #3092. So I've pushed a new commit that removes that segment and it's associated comment.

@redpolline
Copy link
Contributor Author

redpolline commented Feb 23, 2024

Please do not modify the CI files, there is no reason to do so as it works perfectly fine right now.

There are two reasons that I did so:

  1. The version of VS installed on the Windows CI is out of date.

This is the reason why this PR has multiple force-pushes against it. The version of Boost in Vita3K's external/ expects vs143 for it to be deemed "usable" by Boost's own cmake files, but the version installed on the CI is vs140. As such, cmake rejects the Boost in external/ and fails the build attempt unless we force it to use the Boost installed by vcpkg. I assume this was the expectation anyway given that the Windows CI was explicitly configured to download and install Boost via vcpkg. Forcing this to happen requires enabling VITA3K_FORCE_SYSTEM_BOOST and setting Boost_ROOT to the vcpkg install directory.

As this is an intentional design decision with Vita3K's CI, and not an issue with vcpkg in general, I decided to place those options in the CI config so that others could use vcpkg to build Vita3K without forcing them to use a system provided Boost library.

  1. The cmake policies set in CMakeLists.txt are being unset by the Windows CI.

This seems to be a bug with vcpkg. As I ran some tests before and after against the Windows CI above and confirmed that while the set_policy(CMP_0144) command used in CMakeLists.txt did set the policy as instructed, it was unset at some point before / during the call to find_package().

This behavior doesn't occur under any other build environment for any OS. Not even under VS2022 Community. Regardless, we cannot currently expect that the the policies set in CMakeLists.txt will actually be used by the CI. So we have no other means to set them except as an argument to cmake. I can try to remove them, but that means the old deprecated cmake behavior will be re-enabled and warnings will start showing up again in the CI logs. (Assuming that removing CMP_0144 doesn't break the build again. It was required under VS2022 Community, and is also required by the settings I mentioned above. As CMP_0144 controls the behavior of find_package() when *_ROOT is set. The old deprecated behavior of CMP_0144 is to ignore that setting for compatibility with older CMakeLists.txt files.)

@Macdu
Copy link
Contributor

Macdu commented Feb 27, 2024

Using vcpkg is really straightforward, just add the cmake toolchain and use find_package, that's all. There should be no mention of vcpkg in the CMakeList file.
If custom boost is not being used, external/boost shouldn't even be looked at.

About CMP_0144, it was added in 3.27 and we require 3.17, to fix it. You say that if the build system is windows it is at least CMake 3.27 while if it not the cmake version is less than 3.17...
You can just use BOOSTROOT instead of BOOST_ROOT and we will be done (or set the minimum version to 3.17). Also removing CMP_0144 won't break anything, it's just that Boost was already using BOOST_ROOT in its cmake file before this policy was introduced.

The vc140 issue is absolutely not because of the CI being out of date (it is always up to date which can be kind of annoying if vs suddenly decides to break something) but because of this https://github.com/microsoft/vcpkg/blob/3dd44b931481d7a8e9ba412621fa810232b66289/scripts/buildsystems/vcpkg.cmake#L825 vcpkg is assuming that if boost is installed we let him do its stuff which is not the best but fine except if you do weird not useful things like in this PR.

@redpolline
Copy link
Contributor Author

redpolline commented Feb 29, 2024 via email

Copy link
Contributor

@Zangetsu38 Zangetsu38 left a comment

Choose a reason for hiding this comment

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

i have tested after clean my build folder broken again build
And now is fixed, no any boost error in link 👌

@Macdu
Copy link
Contributor

Macdu commented Mar 4, 2024

Don't set a different minimum boost version depending on whether you are using Windows or Linux, this makes no sense. Just use BOOSTROOT instead of BOOST_ROOT and the warning will go away.

Also as I said the default behavior is:

  • look if cmake is able to find itself a boost distribution
  • if it isn't able, build boost yourself
    There should be no reason to have to modify the workflow file for this.

Finally the boost custom build makes both the debug and release version. It should really have no issue. Using VITA3K_Boost_FOUND and other stuff sounds really hacky and unnecessary in this case. CMake can be bad but not to this point and same can be said about Boost.

@redpolline
Copy link
Contributor Author

redpolline commented Mar 11, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to the build process .github
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build broken with boost lib
3 participants