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

PREFIX doesn't go through C preprocessor stringification #57984

Merged
merged 4 commits into from
Jun 6, 2022

Conversation

alef
Copy link
Contributor

@alef alef commented May 28, 2022

Summary

None

Purpose of change

Fixes #57964

Describe the solution

See "Additional context" in the related issue: to use the same tecnique as for version.h.in -> version.h

Describe alternatives you've considered

None

Testing

Built with CMake and Makefile. CI will verify the other platforms.

Additional context

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels May 28, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label May 28, 2022
@ZhilkinSerg ZhilkinSerg merged commit 4f1c3fc into CleverRaven:master Jun 6, 2022
bombasticSlacks pushed a commit to bombasticSlacks/Cataclysm-DDA that referenced this pull request Jun 10, 2022
…#57984)

* Apply -Wno-unknown-warning-option to clang only

* Remove GNU C++ extensions

* Define PREFIX in prefix.h. Similar as done with version.h

Removes the Q/QUOTE stringify macros

* Fix cmake-lint
@perryprog
Copy link
Contributor

Hm, seems like this change broke compilation for me using Apple clang version 13.1.6 and on normal clang version 13.0.1. (M1 MacBook Pro). The following flags are getting marked as unknown:

[1/360] Building CXX object src/CMakeFiles/cataclysm-tiles.dir/cmake_pch_arm64.hxx.pch
FAILED: src/CMakeFiles/cataclysm-tiles.dir/cmake_pch_arm64.hxx.pch
ccache /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DBACKTRACE -DCMAKE -DDATA_DIR_PREFIX -DGIT_VERSION -DMACOSX -DOSX_SDL2_LIBS -DPREFIX=/usr/local -DRELEASE -DSDL_SOUND -DTILES -DUSE_HOME_DIR -I/Users/perry/git/cataclysm-dda/src -I/opt/homebrew/include/SDL2 -isystem /Users/perry/git/cataclysm-dda/src/third-party -Werror -Wall -Wextra -Wformat-signedness -Wlogical-op -Wmissing-declarations -Wmissing-noreturn -Wnon-virtual-dtor -Wold-style-cast -Woverloaded-virtual -Wpedantic -Wsuggest-override -Wunused-macros -Wzero-as-null-pointer-constant -Wno-unknown-warning -Wredundant-decls -Wno-unused-variable -fsigned-char -O3 -DNDEBUG -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk -pthread -std=c++14 -Winvalid-pch -Xclang -emit-pch -Xclang -include -Xclang /Users/perry/git/cataclysm-dda/build/src/CMakeFiles/cataclysm-tiles.dir/cmake_pch_arm64.hxx -x c++-header -MD -MT src/CMakeFiles/cataclysm-tiles.dir/cmake_pch_arm64.hxx.pch -MF src/CMakeFiles/cataclysm-tiles.dir/cmake_pch_arm64.hxx.pch.d -o src/CMakeFiles/cataclysm-tiles.dir/cmake_pch_arm64.hxx.pch -c /Users/perry/git/cataclysm-dda/build/src/CMakeFiles/cataclysm-tiles.dir/cmake_pch_arm64.hxx.cxx
error: unknown warning option '-Wformat-signedness' [-Werror,-Wunknown-warning-option]
error: unknown warning option '-Wlogical-op'; did you mean '-Wlong-long'? [-Werror,-Wunknown-warning-option]
error: unknown warning option '-Wno-unknown-warning'; did you mean '-Wno-unknown-argument'? [-Werror,-Wunknown-warning-option]

Manually adding -Wno-unknown-warning-option to CATA_OTHER_FLAGS makes it happy, however.

I don't see anyone else mentioning this in recent issues, so it could be an issue with my setup, but curious if you have any thoughts alef.

@anothersimulacrum
Copy link
Member

-Wno-unknown-warning-option is automatically set in the Makefile build. Further discussion should go in an issue, stuff on merged PRs gets lost.

@perryprog
Copy link
Contributor

perryprog commented Jun 18, 2022

Sorry, should've mentioned this was CMake, which doesn't seem to set that (anymore?). Not able to make an issue at the moment, but I should be able to tomorrow.

Edit: just took a proper look at the change this made, and yeah, it's not set anymore so I assume the if condition isn't testing what it should be. I'll see if I can make a PR tomorrow that'll fix it.

Comment on lines +245 to +246
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to break ncurses build on msys2 by causing the ghc filesystem files to give undefined symbol errors. Since this is a very edge case I'm not sure whether it's a problem with msys2, ghc filesystem, or cmake.

@alef alef deleted the 57964-prefix branch September 10, 2022 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong base_path_value when path contains C++ macros
5 participants