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

[C++] do not hardcode Unix Makefiles generator #2745

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

mkmkme
Copy link
Contributor

@mkmkme mkmkme commented Feb 17, 2024

Instead, use cmake --build. It has been mentioned at least in cmake 3.2 documentation 1 which has been released in 2015. This will make a build process more flexible to people who want to use different build systems such as Ninja.

What is the purpose of the change

This change makes the build process slightly more flexible to people who use different build systems.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Documentation

  • Does this pull request introduce a new feature? no

@github-actions github-actions bot added C++ Pull Requests for C++ binding build labels Feb 17, 2024
@mkmkme
Copy link
Contributor Author

mkmkme commented Feb 17, 2024

Sorry for the lack of Jira issue. I'm not sure how to create one.

Also I tested this at least locally, and all the affected targets work fine for me. Please tell me if this breaks anything for you (it shouldn't though)

@@ -71,7 +71,6 @@ function do_dist() {
fi
}

(mkdir -p build; cd build; cmake --version; cmake -G "Unix Makefiles" ..)
Copy link
Member

Choose a reason for hiding this comment

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

The clean target fails if another target that creates the build/ folder is not executed first.

avro/lang/c++ on  mkmkme/no-make [$?] via △ v3.22.1 
❯ ./build.sh clean
Error: /home/martin/git/apache/avro/lang/c++/build is not a directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah true! Thanks, I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a configuration step before the loop

Instead, use `cmake --build`. It has been mentioned at least in cmake
3.2 documentation [1] which has been released in 2015.
This will make a build process more flexible to people who want to use
different build systems such as Ninja.

[1]: https://cmake.org/cmake/help/v3.2/manual/cmake.1.html
@martin-g martin-g merged commit c66e8ca into apache:main Feb 19, 2024
4 checks passed
martin-g pushed a commit that referenced this pull request Feb 19, 2024
Instead, use `cmake --build`. It has been mentioned at least in cmake
3.2 documentation [1] which has been released in 2015.
This will make a build process more flexible to people who want to use
different build systems such as Ninja.

[1]: https://cmake.org/cmake/help/v3.2/manual/cmake.1.html

(cherry picked from commit c66e8ca)
@martin-g
Copy link
Member

Thank you, @mkmkme !

@mkmkme mkmkme deleted the mkmkme/no-make branch February 19, 2024 09:41
RanbirK pushed a commit to RanbirK/avro that referenced this pull request May 13, 2024
Instead, use `cmake --build`. It has been mentioned at least in cmake
3.2 documentation [1] which has been released in 2015.
This will make a build process more flexible to people who want to use
different build systems such as Ninja.

[1]: https://cmake.org/cmake/help/v3.2/manual/cmake.1.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build C++ Pull Requests for C++ binding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants