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.

Sign up for GitHub

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

Jump to bottom

Add: [CMake] Option to only build tools #8372

Merged
merged 1 commit into from Dec 13, 2020
Merged

Add: [CMake] Option to only build tools #8372

merged 1 commit into from Dec 13, 2020

Conversation

glx22
Copy link
Contributor

@glx22 glx22 commented Dec 12, 2020

I first tried adding if(NOT OPTION_TOOLS_ONLY) around parts of CMakeLists.txt, but I finally opted for something less invasive.
Of course there's some copy/paste (add_custom_target and add_subdirectory), but it's cleaner for me that way.

TrueBrain
TrueBrain approved these changes Dec 13, 2020
View reviewed changes
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Nice, simple, clean. Sweet :)

TrueBrain
TrueBrain requested changes Dec 13, 2020
View reviewed changes
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

With this change, the Mac OS builds don't work:
/Users/runner/work/OpenTTD/OpenTTD/src/settingsgen/../3rdparty/optional/optional.hpp:182:27: error: unknown type name 'constexpr'

No clue what it is on about .. but possibly we skip too much now.

@TrueBrain
Copy link
Member

Found it: we now skip this too:

if(MSVC)
    # C++17 for MSVC
    set(CMAKE_CXX_STANDARD 17)
else()
    # C++11 for all other targets
    set(CMAKE_CXX_STANDARD 11)
endif()

set(CMAKE_CXX_STANDARD_REQUIRED YES)
set(CMAKE_CXX_EXTENSIONS NO)

set(CMAKE_EXPORT_COMPILE_COMMANDS YES)

and later too:

add_endian_definition()

and below that:

include(CompileFlags)
compile_flags()

after that:

if(IPO_FOUND)
    set_target_properties(openttd PROPERTIES INTERPROCEDURAL_OPTIMIZATION_RELEASE True)
    set_target_properties(openttd PROPERTIES INTERPROCEDURAL_OPTIMIZATION_MINSIZEREL True)
    set_target_properties(openttd PROPERTIES INTERPROCEDURAL_OPTIMIZATION_RELWITHDEBINFO True)
endif()
set_target_properties(openttd PROPERTIES VS_DEBUGGER_WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}/bin")
process_compile_flags()

if(APPLE OR UNIX)
    add_definitions(-DUNIX)
endif()

and almost at the end:

if(CMAKE_SIZEOF_VOID_P EQUAL 8)
    add_definitions(-D_SQ64)
endif()

I think those are the important blobs. So we might want to re-arrange some things, or maybe better, move them to their own files? Sorry ... this was not as simple as you might have hoped :D

@glx22
Copy link
Contributor Author

glx22 commented Dec 13, 2020

Added OPTION_DOCS_ONLY.
script/api/CMakeLists.txt is kind of ugly because I wanted only the required targets. Keeping all script_api targets is possible (and should be cleaner).

@glx22
Copy link
Contributor Author

glx22 commented Dec 13, 2020

Merging both options seems to be the cleanest solution. I'm just not sure about the option name.

TrueBrain
TrueBrain reviewed Dec 13, 2020
View reviewed changes
cmake/Options.cmake Outdated
@@ -57,6 +57,7 @@ function(set_options)
option(OPTION_USE_ASSERTS "Use assertions; leave enabled for nightlies, betas, and RCs" ON)
option(OPTION_USE_THREADS "Use threads" ON)
option(OPTION_USE_NSIS "Use NSIS to create windows installer; enable only for stable releases" OFF)
option(OPTION_TOOLS_DOCS "Build only tools and docs targets" OFF)
Copy link
Member

Choose a reason for hiding this comment

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

weird name for the option honestly. I wouldn't guess what it does based on that. Can't we consider "docs" to be a tool too? OPTION_TOOLS_ONLY sounded so much better :) Maybe others have good ideas :)

TrueBrain
TrueBrain approved these changes Dec 13, 2020
View reviewed changes
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

\o/

Seriously nice job, tnx a lot for putting in the effort :)

@glx22 glx22 merged commit d1fa6b1 into OpenTTD:master Dec 13, 2020
@glx22 glx22 deleted the tools branch December 13, 2020 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TrueBrain TrueBrain TrueBrain approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@glx22 @TrueBrain