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

Initial CMake build support #252

Merged
merged 23 commits into from
Feb 4, 2025
Merged

Initial CMake build support #252

merged 23 commits into from
Feb 4, 2025

Conversation

externl
Copy link
Member

@externl externl commented Feb 3, 2025

This PR adds the initial support for building the C++ demos using CMake. The end goal is to use replace the Make and MSBuild based build systems with this.

Currently I've only added support for greeter and greeterAsync.

We're using standard cmake setup which is different from our usual Make and MSBuild build systems. The compiled binaries will be available in the build directory (in a nested Release/Debug folder for multi config build systems).

Configure cmake:

cd Ice/greeter/
mkdir build
cd build
cmake ..

Build:

cmake --build .

Note:

Ice/greeter uses a helper function I wrote (similar to one protobuf has)
Ice/greeterAsync does the slice compilation "by hand".

I think having this function will be useful for more complicated builds. I'm undecided if we should use it in our demos.

Note 2:

The build does not include lots of compiler build flags as it's not a best practice as they're often highly compiler dependent and are unnecessary for users. See https://alexreinking.com/blog/how-to-use-cmake-without-the-agonizing-pain-part-2.html

In the future we can add a profile for us to use for building with all of the warnings enabled.

EDIT:

  • Removed the top level CMakeLists.txt. It was just making things more painful.
  • Always use the helper function

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 6 changed files in this pull request and generated no comments.

Files not reviewed (5)
  • .vscode/settings.json: Language not supported
  • cpp/CMakeLists.txt: Language not supported
  • cpp/Ice/greeter/CMakeLists.txt: Language not supported
  • cpp/Ice/greeterAsync/CMakeLists.txt: Language not supported
  • cpp/cmake/common.cmake: Language not supported
"gradle.nestedProjects": true,
"cmake.sourceDirectory": "${workspaceFolder}/cpp",
"cmake.configureSettings": {
"Ice_HOME": "${workspaceFolder}/../ice",
Copy link
Member Author

Choose a reason for hiding this comment

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

This sets the cmake variable Ice_HOME to our usual "expected" location, adjacent to the ice-demos dir. This way you don't need to open visual studio code with the correct environment.

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/cmake/common.cmake Show resolved Hide resolved
cpp/cmake/common.cmake Show resolved Hide resolved
cpp/cmake/common.cmake Show resolved Hide resolved
cpp/Ice/greeterAsync/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/Ice/greeterAsync/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/cmake/common.cmake Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 9 changed files in this pull request and generated no comments.

Files not reviewed (5)
  • .vscode/settings.json: Language not supported
  • cpp/Ice/greeter/CMakeLists.txt: Language not supported
  • cpp/Ice/greeterAsync/CMakeLists.txt: Language not supported
  • cpp/Ice/secure/CMakeLists.txt: Language not supported
  • cpp/cmake/common.cmake: Language not supported
Comments suppressed due to low confidence (2)

cpp/Ice/greeter/README.md:18

  • The line '-server' should be removed completely to avoid confusion.
-server

cpp/Ice/greeter/README.md:32

  • The line '-client' should be removed completely to avoid confusion.
-client

```shell
cmake -B build
cmake --build build --config Release
Copy link
Member

Choose a reason for hiding this comment

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

The default is Debug? Should we just show the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I read cmake doesn't have a default and it's all about the generator's default which apparently in several cases is debug.

**Windows:**

```shell
./build/Release/server
Copy link
Member

Choose a reason for hiding this comment

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

Should this be:

build\Release\server

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Member Author

@externl externl Feb 4, 2025

Choose a reason for hiding this comment

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

Note that this pathing is really generator specific and not OS specific. But since we're showing the defaults which are Make and "Visual Studio" I think it's fine to phrase like this.

cpp/Ice/greeter/README.md Outdated Show resolved Hide resolved
**Windows:**

```shell
./build/Release/client
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
./build/Release/client
.\build\Release\client

cpp/cmake/common.cmake Show resolved Hide resolved
Co-authored-by: Jose  <pepone@users.noreply.github.com>
@externl externl merged commit 561f229 into zeroc-ice:main Feb 4, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants