-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
base: master
Are you sure you want to change the base?
Conversation
Since the upstream has no willing to fix this (boostorg/cobalt#215), I prefer to make this changes here. |
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? |
It is not necessary to patch each individiual check. It is enough to raise a fatal error instead of the top-level |
This would require
This doesn't make sense IMO. |
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. |
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 |
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 (What we would really need in |
Anyway, this will not be implemented in this PR. |
If the compiler does not support it, should we keep the return and add an error message before the return? |
Already do that. |
The patch appears to have simply removed the return and replaced it with an error message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping all nitpicks.
Correct, the origin logic is just return and pass configure step, which should be throw an error message instead, which is my changes. |
@JackBoosY, all boost ports must be updated through the To correctly add a patch to a boost port:
|
never mind that. |
include(cmake/CheckRequirements.cmake) | ||
if (NOT BOOST_COBALT_REQUIREMENTS_MATCHED) | ||
- return() | ||
+ message(FATAL_ERROR "The current compiler is not supported.") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
Fixes #40290 #40310 #39573 #39495 #38498