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

Support cmake 3.8 and higher only #7801

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

ilya-lavrenov
Copy link
Contributor

@ilya-lavrenov ilya-lavrenov commented Jan 26, 2023

  • Removed legacy cmake files, because they don't even work
  • Actualized minimum cmake version for root CMakeLists.txt file and now it becomes 3.8
    • Most recent user feature is target_compile_features with cxx_std_17 or cxx_std_11 arguments
    • PCH files (requiring cmake 3.16) are optional already and it's not a mandatory to build the product.
  • Minor improvements to cmake files
  • Tested with cmake 3.8, 3.14 and 3.25.2

@google-cla
Copy link

google-cla bot commented Jan 26, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added c++ codegen Involving generating code from schema golang and removed golang c++ codegen Involving generating code from schema labels Jan 26, 2023
@ilya-lavrenov
Copy link
Contributor Author

Hi @dbaileychess,
Could you please have a look at the PR? The change is quite simple

@dbaileychess
Copy link
Collaborator

@ilya-lavrenov I left 2 comments the other day that haven't been resolved yet.

@ilya-lavrenov
Copy link
Contributor Author

@ilya-lavrenov I left 2 comments the other day that haven't been resolved yet.

hm, I don't see any comments in the PR..

@dbaileychess
Copy link
Collaborator

oh, that's a stupid change by github. My comments show up in my view but have a new pending chirp that seems new to me.

src/idl_gen_go.cpp Outdated Show resolved Hide resolved
CMake/CMakeLists_legacy.cmake.in Outdated Show resolved Hide resolved
@github-actions github-actions bot added c++ codegen Involving generating code from schema labels Jan 28, 2023
@ilya-lavrenov ilya-lavrenov force-pushed the cmake-3-13-fixes branch 2 times, most recently from 83fc26d to d80fb57 Compare January 28, 2023 19:41
@github-actions github-actions bot removed codegen Involving generating code from schema c++ labels Jan 28, 2023
@ilya-lavrenov ilya-lavrenov changed the title Fixed legacy cmake for < 3.16 version Support cmake 3.8 and higher only Jan 28, 2023
@ilya-lavrenov
Copy link
Contributor Author

ilya-lavrenov commented Jan 28, 2023

@dbaileychess please, have a look.

Now, cmake files look generic enough to support cmake version 3.8 and higher, which allowed to remove legacy non-working cmake script. I've explicitly mentioned places why we need at least 3.8.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
benchmarks/CMakeLists.txt Show resolved Hide resolved
@dbaileychess
Copy link
Collaborator

Thanks, this is a better approach than fixing the legacy. Just a few comments.

@dbaileychess dbaileychess merged commit ca71fdf into google:master Jan 30, 2023
@dbaileychess
Copy link
Collaborator

Thanks, I appreciate the fast turn around!

@ilya-lavrenov ilya-lavrenov deleted the cmake-3-13-fixes branch January 30, 2023 07:23
dbaileychess pushed a commit that referenced this pull request Mar 15, 2023
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
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.

2 participants