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

[service] Update Windows SDK on CI nodes #13159

Closed
HappySeaFox opened this issue Sep 26, 2022 · 18 comments
Closed

[service] Update Windows SDK on CI nodes #13159

HappySeaFox opened this issue Sep 26, 2022 · 18 comments
Assignees
Labels
infrastructure Waiting on tools or services belonging to the infra look into

Comments

@HappySeaFox
Copy link
Contributor

HappySeaFox commented Sep 26, 2022

Blocks #13000

Compilation of my recipe on the CI fails with syntax errors: https://c3i.jfrog.io/c3i/misc/logs/pr/13000/19-configs/windows-visual_studio/sail/0.9.0-rc1//3158f96973765e62212b729a24065a25fb6a30d1-build.txt

C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(487,17): error C2059: syntax error: '/' (compiling source file C:\J\w\prod\BuildSingleReference.conan\data\sail\0.9.0-rc1__\build\3158f96973765e62212b729a24065a25fb6a30d1\src\src\libsail-common\log.c) [C:\J\w\prod\BuildSingleReference.conan\data\sail\0.9.0-rc1__\build\3158f96973765e62212b729a24065a25fb6a30d1\build\src\src\libsail-common\sail-common.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(502,17): warning C5103: pasting '/' and '/' does not result in a valid preprocessing token (compiling source file C:\J\w\prod\BuildSingleReference.conan\data\sail\0.9.0-rc1__\build\3158f96973765e62212b729a24065a25fb6a30d1\src\src\libsail-common\log.c)

Similar issue: microsoft/vcpkg#15035, #10999

Maybe it's an issue with the Windows SDK version installed on the CI nodes? Quote from the vcpkg bugtracker:

This issue should be fixed in Windows SDK 10.0.18362.0 and later versions. please update the version of your Windows SDK.

@uilianries uilianries added look into infrastructure Waiting on tools or services belonging to the infra labels Sep 27, 2022
@HappySeaFox
Copy link
Contributor Author

HappySeaFox commented Sep 29, 2022

As per https://github.com/conan-io/conan-center-index/blob/master/docs/supported_platforms_and_configurations.md#windows, Windows SDK version of the CI nodes is 10.0.20348. However, from the log file I see that 10.0.17763.0 was selected for some reason.

@uilianries
Copy link
Member

@HappySeaFox that markdown is updated manually, it can be outdated.

@HappySeaFox
Copy link
Contributor Author

Blocks #13245

@HappySeaFox
Copy link
Contributor Author

Indirectly blocks #13406

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 24, 2022

Maybe also #13696

@HappySeaFox
Copy link
Contributor Author

Guys, any progress?

@uilianries
Copy link
Member

@HappySeaFox The SDK is installed. Now it need to be configured in Windows machines. It should happen during this week.

@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 19, 2022

Any progress? Windows agents still run SDK 10.0.17763.0 #13285 (comment)

@uilianries
Copy link
Member

@SpaceIm not yet, but it's listed to be done. Right now, we are working on Mac nodes, to be able to run more than one version. It should solve the problem related to CI bottleneck with Mac.

uilianries added a commit to bincrafters/conan-center-index that referenced this issue Dec 21, 2022
Adopt a workaround provided by @jcar87, which is a temporary
hotfix until having conan-io#13159
fixed.

Signed-off-by: Uilian Ries <uilianries@gmail.com>
conan-center-bot pushed a commit that referenced this issue Dec 21, 2022
* Add version 3.0.6

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Remove cmake file

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* missing folder

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Fixes jasper 3.0.6

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* patch 2.0.33

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* requires conan 1.52.0

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* use c99 when cross-building

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* fix cross-building

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Add Ninja to avoid sdk

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Ninja is not mandatory

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Remove duplicated test_package.c

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Links pthread

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Use safe deletion

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Remove Ninja

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate cmake defs

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate cmake alias variables

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Update recipes/jasper/all/test_package/CMakeLists.txt

Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>

* cleanup & small improvements

* deterministic libname

* Use latest Windows SDK available

Adopt a workaround provided by @jcar87, which is a temporary
hotfix until having #13159
fixed.

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Fix toolchain position variables

Signed-off-by: Uilian Ries <uilianries@gmail.com>
Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
@prince-chrismc
Copy link
Contributor

I think this can be closed since Jasper was able to merge 🚀

@HappySeaFox
Copy link
Contributor Author

HappySeaFox commented Dec 22, 2022

I think it's not a good idea. I opened the issue not because of jasper but because of my project, sail, which also wasn't able to build. Any current or future C99 project will fail. When you try to bump its dependencies for example. Closing the issue basically means that you will need to copy-paste the workaround across the repository for any C99 project.

@uilianries
Copy link
Member

@HappySeaFox This has been investigated and many more information was found by @jcar87, but in summary, we have some internal options, like, updating conan global conf to enforce CMake to use the latest SDK version available in the CI machine. Meanwhile, any recipe can use the very similar workaround used here. I'll open a PR to your pull request, adding such piece of code.

@jcar87
Copy link
Contributor

jcar87 commented Dec 22, 2022

We have identified the underlying issue and are working on deploying a fix to all agents that will make the workaround unnecessary.

The correct SDK version is (and always was!) installed on all Windows build agents, the issue here is related to how CMake chooses the SDK version for the "Visual Studio" generators. The way this is done by CMake is not documented (see here, it mentions that it "chooses" a version, but it doesn't provide any details as to how it makes the decision). In our internal testing we were getting different results on different machines with the same SDK versions installed - until we understood what the issue was.

We are currently working in deploying a more permanent fix. Once that is done, we will update and eventually close this issue. Thanks everyone for your patience!

@HappySeaFox
Copy link
Contributor Author

I think filing a bug to the cmake bugtracker is a good idea as well.

@HappySeaFox
Copy link
Contributor Author

Maybe it's a silly question, but is it possible to simply uninstall the old SDK ?

@uilianries
Copy link
Member

uilianries commented Dec 27, 2022

Maybe it's a silly question, but is it possible to simply uninstall the old SDK ?

That's our first idea, uninstalling all old versions, but it's not just a simple desktop machine. We would need to update multiple CI machines and update some configuration scripts too. Indeed it will be done in the future, but as workaround, we are forcing Cmake meanwhile. Good catch bringing us that idea!

@jcar87
Copy link
Contributor

jcar87 commented Jun 1, 2023

I believe this is already fixed - will test this on: #17788 and close this issue once we verify it's resolved.

@jcar87 jcar87 closed this as completed Jun 1, 2023
@jcar87
Copy link
Contributor

jcar87 commented Jul 20, 2023

I think filing a bug to the cmake bugtracker is a good idea as well.

A bit of an update on this - it looks like this behaviour is already fixed in CMake 3.27, and there's a new policy for the Visual Studio generators to use the latest version of the Windows SDK available. The problem we experienced is mentioned in the documentation as well: https://cmake.org/cmake/help/latest/policy/CMP0149.html#policy:CMP0149

In essence, the version of Windows that CMake was running on was potentially anchoring the SDK to an old version. This is more likely to be an issue on Windows Server than it is on Windows 10/11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Waiting on tools or services belonging to the infra look into
Projects
None yet
Development

No branches or pull requests

6 participants