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

[boost-cobalt] Throw error in the configure step when the compiler is not supported #42738

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Dec 16, 2024

@JackBoosY
Copy link
Contributor Author

Since the upstream has no willing to fix this (boostorg/cobalt#215), I prefer to make this changes here.

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Dec 16, 2024

Discussion: as I commented on Discord, should we add a vcpkg function to pre-check the current compiler whether it is supported for the current port?

@dg0yt
Copy link
Contributor

dg0yt commented Dec 16, 2024

It is not necessary to patch each individiual check. It is enough to raise a fatal error instead of the top-level return(), https://github.com/boostorg/cobalt/blob/cd2ea28a53e1f4f9f77ff962207b95358c2074db/CMakeLists.txt#L17

@WangWeiLin-MV WangWeiLin-MV added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Dec 16, 2024
@dg0yt
Copy link
Contributor

dg0yt commented Dec 16, 2024

should we add a vcpkg function to pre-check the current compiler whether it is supported for the current port?

This would require

  • to run CMake before the actual build
  • to duplicate the package's checks.

This doesn't make sense IMO.
(What still makes sense is better forwarding of error message from configuration to the console. But that is a different, old, story.)

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Dec 16, 2024

This would require

  • to run CMake before the actual build
  • to duplicate the package's checks.

This doesn't make sense IMO. (What still makes sense is better forwarding of error message from configuration to the console. But that is a different, old, story.)

Although the compiler will be checked twice, this will avoid returning errors after several minutes or even tens of minutes of build time.

The way I think about it, the compiler will be checked before all the port build, and first check if all the ports in the build list support this compiler version, just like if triplet is supported.

@dg0yt
Copy link
Contributor

dg0yt commented Dec 16, 2024

Although the compiler will be checked twice, this will avoid returning errors after several minutes or even tens of minutes of build time.

Checking during CMake configuration is before the build, not after several minutes.

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Dec 16, 2024

Although the compiler will be checked twice, this will avoid returning errors after several minutes or even tens of minutes of build time.

Checking during CMake configuration is before the build, not after several minutes.

Not needed, this step can be executed in vcpkg binary, the port vcpkg.json can add a new keyword such like compiler: msvc>=x.y.z, gcc>=a.b.c.

@dg0yt
Copy link
Contributor

dg0yt commented Dec 17, 2024

Not needed, this step can be executed in vcpkg binary, the port vcpkg.json can add a new keyword such like compiler: msvc>=x.y.z, gcc>=a.b.c.

The compiler ID and and version knowledge is in the CMake toolchain (project mode), not even in the triplet files (script mode). It might be possible to pull some properties into the tool while hashing the compiler binary, but I don't think it is comming soon. I also wonder how well this would scale over all possible compilers and versions. Once somebody adds a fancy-c++>=1.2.3, most of us will be unable to update this expression.

(What we would really need in supports is the deployment target (android-24, macOS 14.4, Windows 10). At least this is responsible for a lot of ci.baseline.txt entries.)

@JackBoosY
Copy link
Contributor Author

The compiler ID and and version knowledge is in the CMake toolchain (project mode), not even in the triplet files (script mode). It might be possible to pull some properties into the tool while hashing the compiler binary, but I don't think it is comming soon. I also wonder how well this would scale over all possible compilers and versions. Once somebody adds a fancy-c++>=1.2.3, most of us will be unable to update this expression.

(What we would really need in supports is the deployment target (android-24, macOS 14.4, Windows 10). At least this is responsible for a lot of ci.baseline.txt entries.)

Anyway, this will not be implemented in this PR.

@jimwang118
Copy link
Contributor

If the compiler does not support it, should we keep the return and add an error message before the return?

@JackBoosY
Copy link
Contributor Author

If the compiler does not support it, should we keep the return and add an error message before the return?

Already do that.

@jimwang118
Copy link
Contributor

Already do that.

The patch appears to have simply removed the return and replaced it with an error message.

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Skipping all nitpicks.

@JackBoosY
Copy link
Contributor Author

The patch appears to have simply removed the return and replaced it with an error message.

Correct, the origin logic is just return and pass configure step, which should be throw an error message instead, which is my changes.

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Dec 17, 2024
@JavierMatosD
Copy link
Contributor

@JackBoosY, all boost ports must be updated through the scripts/boost/generate-ports.ps1

To correctly add a patch to a boost port:

  1. Place the patch file in the port’s directory.
  2. Bump the port’s version in the $portVersions section of the generate-ports.ps1 script.
  3. Execute the script to regenerate the portfiles.

@JavierMatosD JavierMatosD marked this pull request as draft December 17, 2024 19:58
@JackBoosY
Copy link
Contributor Author

JackBoosY commented Dec 18, 2024

@JavierMatosD Use this command will remove port-version in vcpkg.json, so I didn't commit that.

never mind that.

@JackBoosY JackBoosY marked this pull request as ready for review December 18, 2024 12:22
include(cmake/CheckRequirements.cmake)
if (NOT BOOST_COBALT_REQUIREMENTS_MATCHED)
- return()
+ message(FATAL_ERROR "The current compiler is not supported.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break users who vcpkg install boost with a compiler without these features. I think the boost port should drop the dependency on boost-cobalt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? boost-cobalt is still a part of full boost. If the compiler is not supported, I think throw configure error is expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Throw-error is expected for boost-cobalt, but not so much for an uninformed vcpkg install boost. An optional feature might be a middle way, as already exists for mpi.

@JavierMatosD JavierMatosD marked this pull request as draft December 19, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[boost-cobalt] Build error on x64-linux
5 participants