-
Notifications
You must be signed in to change notification settings - Fork 94
Updated CMake commands to reflect that ENABLE_UNITY_BUILDS now defaults to ON in the AWS SDK #156
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
Conversation
|
I feel that the description of this flag in the official docs comes across as slightly confusing. The way I understand it, it is a built in global setting that can only be changed if it is exposed as a settable option, and the default setting is off. It seems to be a convention in CMake for options to be off by default unless a different default is specified:
|
ci/codebuild/build.sh
Outdated
@@ -6,6 +6,6 @@ set -euo pipefail | |||
cd $CODEBUILD_SRC_DIR | |||
mkdir build | |||
cd build | |||
cmake .. -GNinja -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX=/install $@ | |||
cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX=/install $@ |
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.
Building the project as a shared library is important here, because it brings out any potential linking errors. If we build as a static library instead, linking errors would show up later when the final executable is built. So, for CI we definitely need to be building as a shared library.
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.
I changed the commit to expose BUILD_SHARED_LIBS
as a configurable option.
0d53291
to
2e1aa03
Compare
Honestly, I'm somewhat perplexed, because the It's just that various sources indicate that it (supposedly) must or should be set as an |
I'd argue that you should close this PR and make a new one with the simple change of making |
It seems like |
But it is OFF by default right now. |
I apologize sincerely for having made a mess of this process. What this pull request currently does:
And also:
I've asked on the CMake discussion forum for input on this last point. I completely agree that declaring I'd like to get back to this PR once I have a better understanding of the nuances. |
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.
Reduce the various README changes to only include the -DENABLE_UNITY_BUILD=ON
corrections
2e1aa03
to
b3d045b
Compare
…ts to ON in the AWS SDK
This pull request makes a few changes to the READMEs. In particular:
ENABLE_UNITY_BUILD
defaults toON
since aws/aws-sdk-cpp@2d17946BUILD_SHARED_LIBS
build optionBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.