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

CI: complete the Windows CI coverage #832

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

compnerd
Copy link
Contributor

Extend the Windows CI to include the tests as well. This will mostly
complete the Windows support (the remaining pieces remain in supporting
the Windows ARM64 build host).

@compnerd
Copy link
Contributor Author

@keith okay, the install issue is resolved, but now the problem is that Path and SDKROOT are not in the environment, and I don't see how to get that added :(

@keith
Copy link
Member

keith commented Feb 20, 2024

are the values stable enough to pass directly? like --repo_env=ProgramData=whatever

@compnerd
Copy link
Contributor Author

compnerd commented Feb 21, 2024

I suppose that they should be - the installer is installing into a user specific location - %LocalAppData%\Programs\Swift\Toolchains\%SwiftVersion%+Asserts\usr\bin, the runtime is located at %LocalAppData%\Programs\Swift\Runtimes\%SwiftVersion%\usr\bin and SDKROOT can be assumed to be for Windows at %LocalAppData%\Programs\Swift\Platforms\Windows.platform\Developer\SDKs\Windows.sdk.

@compnerd
Copy link
Contributor Author

@keith I'm not sure how to make the appropriate changes for this, so a bit of guidance would be appreciated.

@keith
Copy link
Member

keith commented Feb 23, 2024

I was thinking that if you set that in the environment key like you're doing for the others, and then passed --repo_env=ProgramData it would cover it?

Extend the Windows CI to include the tests as well.  This will mostly
complete the Windows support (the remaining pieces remain in supporting
the Windows ARM64 build host).
@compnerd
Copy link
Contributor Author

I'm not too fond of the approach, but it does get further:

Auto-Configuration Error: Setting up VC environment variables failed, WINDOWSSDKDIR is not set by the following command:
--
  | "C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Auxiliary\Build\VCVARSALL.BAT" amd64  -vcvars_ver=14.37.32822

@keith
Copy link
Member

keith commented Feb 23, 2024

is that error one you're thinking the CI setup should have solved?

@compnerd
Copy link
Contributor Author

Well, that seems like something that general Windows support should have figured out. I'm not sure what is going on, but it seems that we are missing something in the image possibly? Or we are not invoking things properly.

@keith
Copy link
Member

keith commented Feb 23, 2024

this makes it sound like maybe the windows setup doesn't have the SDK? bazelbuild/bazel#13261

is that what you're thinking?

should that be solved by https://github.com/bazelbuild/continuous-integration/blob/master/buildkite/setup-windows.ps1#L137 ?

@compnerd
Copy link
Contributor Author

Yeah, exactly, it seems like that is what is needed.

@keith
Copy link
Member

keith commented Feb 23, 2024

is it possible it's setup correctly but the env var isn't set? i assume that link is the current runner

@brentleyjones
Copy link
Collaborator

@compnerd Wondering where we stand with this. We are going to be making some changes to the worker soon, and I want to make sure I don't regress Windows support.

@compnerd
Copy link
Contributor Author

@brentleyjones I think that @keith is right and that the Windows SDK is missing. Once that is installed, I think that this should just work out of the box now. The admin requirements are removed upstream and we now install to the user's directory. We no longer need to modify system files so that is also not an issue. As long as we have the SeCreateSymbolicLinkPrivilege privilege (or are running as admin), we should be able to support this. I'll try to see if I can get this rebased soon so we can finally get it merged.

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.

3 participants