Skip to content

Add C11 and C17 conformance coverage with VS 2017 #1294

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

Merged
merged 8 commits into from
Jun 5, 2023

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Jun 2, 2023

Description

This PR resolves CDRIVER-4549 by adding a vs2017x64 task to the std-matrix EVG variant and resolves CDRIVER-4334 by removing use of the rhel62 variant. Verified by this patch.

windows-64-vs2017 -> windows-vsCurrent

As recommended by the distro guidelines, this PR moves all tasks that require the VS 2017 compiler to the "windows-vsCurrent" distro. Tasks that ran on "windows-64-vs2017-test" in the legacy config were moved to "windows-vsCurrent-large".

The task naming convention uses the Windows Server version for the specified distro rather than vsCurrent to avoid redundancy/confusion about the VS version being used. e.g. the "cse-sasl-cyrus-openssl-vs2017-x64-compile" which ran on windows-64-vs2017 is now named "cse-sasl-cyrus-openssl-windows-2019-vs2017-x64-compile" and runs on "windows-vsCurrent".

This migration required updating the "debug-compile-aws" task to use "find-cmake-latest.sh", as the CMake binary selected by "find-cmake.sh" is broken on the "windows-vsCurrent" distro (cannot find MSBuild.exe during CMake configuration).

VS 2022 Compatibility

Although the C Driver has no issues with VS 2022 compatibility, libmongocrypt needed a patch (106768d) in CSE tasks to address an internal compiler error. Applied a cherry-pick of the required patch to minimize changes relative to the 1.8.0 release, which may be removed once the commit is included in the latest minimum libmongocrypt version (bump from 1.8.0).

RHEL 6.2 Removal

As a drive-by improvement, removed references to the "rhel62" distro as well, which also obviated some test skips.

C11 and C17 Coverage on Windows

The primary goal of this PR. Given we now use "find-latest-cmake.sh" with CMake version 3.25.2, the CMAKE_C_STANDARD workaround is no longer necessary and was thus removed.

Use of /std:cXY was complicated by Windows 10 SDK header files not being standard-conforming. This was worked-around by specifying a patched version of the Windows 10 SDK via CMAKE_SYSTEM_VERSION. Alas not all warnings are addressed, so /wd5105 was also added to compiler flags.

Miscellaneous

  • Updated calls to "compile-libmongocrypt.sh" to include stdout output on failure. This was motivated by the ICE error addressed by mongodb/libmongocrypt@fcf2b5b not being emitted as stderr output.
  • Replaced all calls to GNU Make with cmake --build for platform-independent command consistency as well as to address the occasional absence of the "all" target when using the VS 2017 generator (when/why?).
  • Removed redundant invocation of find_cmake_latest when find-cmake-latest.sh is sourced.

@eramongodb eramongodb requested a review from kevinAlbs June 2, 2023 22:07
@eramongodb eramongodb self-assigned this Jun 2, 2023
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Move to recommended windows-vsCurrent distro, and associated CI improvements are much appreciated. TIL about CMAKE_SYSTEM_VERSION.

@eramongodb eramongodb merged commit 3ca6cf3 into mongodb:master Jun 5, 2023
@eramongodb eramongodb deleted the cdriver-4549 branch June 5, 2023 15:07
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