Skip to content

Commit

Permalink
[LSP]: fix cmake cxx standard required (chapel-lang#23531)
Browse files Browse the repository at this point in the history
This PR changes how `chpldef` sets the required minimum `c++` version
with `cmake`.

When testing locally with cmake version `v3.22.1`, I was experiencing
different behavior than on `chapdl` or `chapcs`, both of which have
different versions of `cmake`. After some digging, it appears there's a
behavior change in `cmake 3.22.0` that introduced a bug causing the
standard not to be set in this case. The bug was later fixed in
`v3.22.2`. See
https://cmake.org/cmake/help/git-stage/release/3.22.html#cmake-3-22-release-notes.

Using a different strategy to set the required version (the same one we
use everywhere else) allows `chpldef` to be built with the required
standard on `cmake` `v3.22.0` and `v3.22.1`, in addition to the previous
and later versions.

Also discovered that `cmake` `v3.26` changed the behavior of adding
compiler flags and setting the `c++` standard such that it reordered the
flags, putting the language standard flag near the beginning instead of
near the end. This creates inconsistent behavior between `cmake`
versions because our build script passes the `c++` std flag in multiple
locations, and earlier versions of `cmake` just happened to work because
the `cmake` generated flag was added after all those. Now, in new
versions, it comes before and it breaks the builds.

The solution should be to use a `cmake` policy to opt out of the new
behavior, but the devs didn't include a policy for this change
because...well...they didn't.

see https://gitlab.kitware.com/cmake/cmake/-/merge_requests/7931

So to work around this new limitation, I started parsing the flags
passed from `make` to remove the `-std=c++14` flag as it's redundant to
all the `cmake` targets, which have logic to specify the correct
standard already. A different option would be to adjust the `Makefile`
to strip the flag, or possibly just adjust the `chpl_llvm.py` script to
stop appending the flag.
  • Loading branch information
arezaii authored Oct 4, 2023
2 parents b61edea + 1beaff0 commit 0ba5534
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 2 deletions.
5 changes: 5 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,8 @@ endif()
# set(DEBUG OFF CACHE BOOL "DEBUG flag from make") # CMake Debug?
# set(PROFILE OFF CACHE BOOL "PROFILE flag from make")

# We set the cxx standard in the sub folders, strip the standard from the bunch of flags we got
string(REPLACE "-std=c++14" "" CHPL_CXX_FLAGS "${CHPL_CXX_FLAGS}")
message(VERBOSE "Using C++ compile options ${CHPL_CXX_FLAGS}")
message(VERBOSE "Using C++ link options ${CHPL_LD_FLAGS}")

Expand Down Expand Up @@ -322,6 +324,9 @@ foreach (CHPLENV_LINE IN LISTS CHPLENV_OUTPUT)
# message(DEBUG "Setting ${CHPLENV_LINE} as ${CHPL_ENV_NAME} ${CHPL_ENV_VALUE}")
set(${CHPL_ENV_NAME} ${CHPL_ENV_VALUE} CACHE STRING "overwritten description" FORCE)
endforeach()
# We set the cxx standard in the sub folders, strip the standard from the bunch of flags we got
string(REPLACE "-std=c++14" "" CHPL_HOST_BUNDLED_COMPILE_ARGS "${CHPL_HOST_BUNDLED_COMPILE_ARGS}")
string(REPLACE "-std=c++14" "" CHPL_HOST_SYSTEM_COMPILE_ARGS "${CHPL_HOST_SYSTEM_COMPILE_ARGS}")

if ((CHPL_HOME STREQUAL CMAKE_BINARY_DIR) OR
(CMAKE_SOURCE_DIR STREQUAL CMAKE_BINARY_DIR))
Expand Down
6 changes: 4 additions & 2 deletions tools/chpldef/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
# See the License for the specific language governing permissions and
# limitations under the License.

# the lanuguage features used in the server require at least c++17
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED True)

# Put all the binary source files into an object blob.
add_library(chpldef-objects
command-line-flags.cpp
Expand All @@ -37,7 +41,6 @@ target_include_directories(chpldef-objects PRIVATE
${CHPL_MAIN_INCLUDE_DIR}
${CHPL_INCLUDE_DIR}
${CMAKE_CURRENT_SOURCE_DIR})
set_property(TARGET chpldef-objects PROPERTY CXX_STANDARD 17)

# Create the 'chpldef' executable.
add_executable(chpldef chpldef.cpp)
Expand All @@ -47,7 +50,6 @@ target_include_directories(chpldef PRIVATE
${CHPL_MAIN_INCLUDE_DIR}
${CHPL_INCLUDE_DIR}
${CHPLDEF_INCLUDE_DIR})
set_property(TARGET chpldef PROPERTY CXX_STANDARD 17)

# Enable testing if the test folder exists.
if (IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/test")
Expand Down

0 comments on commit 0ba5534

Please sign in to comment.