Skip to content

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

Merged
merged 2 commits into from
Aug 24, 2022

Conversation

specious
Copy link
Contributor

This pull request makes a few changes to the READMEs. In particular:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@marcomagdy
Copy link
Contributor

BUILD_SHARED_LIBS is a CMake option. It's available in every CMake project.

@specious
Copy link
Contributor Author

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:

If no initial <value> is provided, boolean OFF is the default value

@@ -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 $@
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@specious specious force-pushed the docs/cmake-commands branch 3 times, most recently from 0d53291 to 2e1aa03 Compare August 19, 2022 21:07
@specious
Copy link
Contributor Author

Honestly, I'm somewhat perplexed, because the BUILD_SHARED_LIBS option appears to have been working according to the build logs.

It's just that various sources indicate that it (supposedly) must or should be set as an option() before you use it.

@marcomagdy
Copy link
Contributor

BUILD_SHARED_LIBS is working for sure as is.

I'd argue that you should close this PR and make a new one with the simple change of making BUILD_SHARED_LIBS set to ON by default.

@specious
Copy link
Contributor Author

It seems like BUILD_SHARED_LIBS should be OFF by default.

@marcomagdy
Copy link
Contributor

But it is OFF by default right now.
I honestly don't know what the point of this PR is right now.

@specious
Copy link
Contributor Author

I apologize sincerely for having made a mess of this process.

What this pull request currently does:

  • Removes -DENABLE_UNITY_BUILD=ON from AWS C++ SDK build command (because it's now been the default for 3 years)
  • Removes -DBUILD_SHARED_LIBS= directive from the commands to build the runtime in the examples
  • Some clarifications in the READMEs

And also:

  • Declares BUILD_SHARED_LIBS as an option()

I've asked on the CMake discussion forum for input on this last point.

I completely agree that declaring BUILD_SHARED_LIBS as an option in the project doesn't appear to carry any functional benefit, however I'd like to see what people who are really familiar with this aspect of CMake have to say.

I'd like to get back to this PR once I have a better understanding of the nuances.

Copy link
Collaborator

@bmoffatt bmoffatt left a 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

@bmoffatt bmoffatt added the documentation Better documentation can fix this label Aug 23, 2022
@specious specious force-pushed the docs/cmake-commands branch from 2e1aa03 to b3d045b Compare August 23, 2022 18:57
@specious specious changed the title Minor edits in the READMEs along with updated CMake commands Updated CMake commands to reflect that ENABLE_UNITY_BUILDS now defaults to ON in the AWS SDK Aug 23, 2022
@bmoffatt bmoffatt merged commit f385f53 into awslabs:master Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Better documentation can fix this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants