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

winPB: Switch to VS2022 build tools and add Windows build dockerfile #3702

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

sxa
Copy link
Member

@sxa sxa commented Aug 8, 2024

Notes:

  • Adds Dockerfile.win2022 which can be used to create a build image comparable to the Linux ones. This will only install VS2022, not the earlier other compilers (this will be fine for all versions once Use VS 2022 by default for all JDK versions + skip freetype patching temurin-build#3907 is merged)
  • Various modifications to support the above including:
    • Adding more 'win_reboot' tags to tasks to avoid attempting to reboot while running ansible inside the container
    • Use of gpg2 instead of gpg since that is what the new cygwin seems to have
    • Since the dockerfile does a base cygwin install, the ansible cygwin role now checks for jq.exe (the most recent addition) instead before deciding whether to skip the cygwin packages
    • Uses a windows command instead of cygwin (which may not be in the path at that point) as the "Dummy" command in the logs role
    • Changes the command used for default shortname creation to one that works in containers
  • Uses Visual Studio Build Tools instead of the community edition (Currently will not take effect if "adoptopenjdk" is skipped but that is skipped in the new dockerfile so it takes effect there)

Not done:

  • I haven't got the git_sha detection working in the logs role, so it is currently hard coded in the Docker file to 00000000. Suggestions on how to make this work are welcome
  • The cygwin.exe checksum is not verified in the Dockerfile
  • If the playbook fails the Docker process does not currently fail as & doesn't propagate the failure

The Dockerfile file can be build with an invocation such as the following (there is a password in the dockerfile, but perhaps that should be removed? The account is deleted afterwards so the password cannot be used once the image is complete):

For discussion from reviewers:

  • Is using gpg2 instead of gpg going to cause any problems (e.g. on existing machines)?
  • I am now checking for the presence of C:\Program Files\Microsoft Visual Studio\2022 instead of the \Community subdirectory so that community or build tools will be detected (Although I've just realised that since build tools goes into C:\Program Files (x64) - as can be seen in the Register Visual Studio Community 2022 DIA SDK shared libraries` operation - so that probably isn't going to work anyway for the build tools case
  • I'd quite like to get this in with the "known issues" that I can raise follow-up issues on so that progress with the automation around this can proceed
  • Should I remove the temporary password from the Dockerfile and force it to be explicitly specified?
  • I'd rather not be using WinRM with a user and just run this in a local manner, but that would require more effort on Windows. Any interested parties can look at those changes for use later

Fixes #3286 adoptium/temurin-build#3787 (Note that for validation purposes I have also run through the process to verify that this dockerfile and compiler can produce a binary identical build of 21.0.4+7 on Windows)

Checklist

…d fixes

Notes:
- Adds Dockerfile.win2022 which can be used to create a build image
  comparable to the Linux ones. This will only install VS2022, not
  the earlier other compilers
- Various modifications to support the above including:
  - Adding more 'win_reboot' tags to tasks to avoid attempting
    to reboot while running ansible inside the container
  - Use of gpg2 instead of gpg since that is what the new cygwin
    seems to have
  - Since the dockerfile does a base cygwin install, the ansible
    cygwin role now checks for jq.exe (the most recent addition)
    instead before deciding whether to skip the cygwin packages
  - Uses a windows command instead of cygwin (which may not be in
    the path at that point) as the "Dummy" command in the logs role
  - Changes the command used for default shortname creation to one
    that works in containers
- Uses Visual Studio Build Tools instead of the community edition
  (Currently will not take effect if "adoptopenjdk" is skipped
  but that is skipped in the new dockerfile so it takes effect there)

Signed-off-by: Stewart X Addison <[email protected]>
@sxa sxa added os:windows secure-dev Issues specific to SSDF/SLSA compliance work labels Aug 8, 2024
@sxa sxa added this to the 2024-08 (August) milestone Aug 8, 2024
@sxa sxa self-assigned this Aug 8, 2024
@sxa sxa marked this pull request as draft August 8, 2024 10:52
@gdams
Copy link
Member

gdams commented Aug 8, 2024

how do you plan to build/push this? A GitHub action might be quite smart?

Co-authored-by: George Adams <[email protected]>
@sxa
Copy link
Member Author

sxa commented Aug 8, 2024

how do you plan to build/push this? A GitHub action might be quite smart?

That's another step beyond this PR so we should discuss elsewhere (perhaps in the related issue). All ideas welcome of course, but I was planning to prototype it on one of our machines under control of jenkins like we do for the others since my preference would not be to rely on GitHub actions and have this done on our own infrastructure.

For initial prototyping with the pipelines we can just have the images locally on the relevant machine(s) as we need to identify a suitable registry to store them in as discussed in the PMC yesterday before we can push them.

Copy link
Contributor

@andrew-m-leonard andrew-m-leonard left a comment

Choose a reason for hiding this comment

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

looks good

Signed-off-by: Stewart X Addison <[email protected]>
@sxa
Copy link
Member Author

sxa commented Aug 12, 2024

Added USER ContainerUser to stop the build running as an Administrator inside the container - I trust this will not invalidate anyone's reviews.

Noting also that the container will need access to a git config which allows the workspace directory to be treated as "safe" due to the initial jenkins git checkout of the repository being done on the host system which has a different SID from the user in the container. I have been doing this by setting HOME in the jenkins agent definition to be C:\jw which is what I'm currently using for the jenkins workspace, and having a .gitconfig in there with the following to allow subsequent git operations in the directory to succeed. (I was trying to work with more restrictive versions of this but haven't got it to work yet)

[safe]
        directory = *

This is not an ideal solution as it would potential allow git configurations to be modified and persisted across the build runs, but that can be investigated later, possibly using a GIT_CONFIG environment variable.

@sxa sxa marked this pull request as ready for review August 12, 2024 14:29
@sxa
Copy link
Member Author

sxa commented Aug 12, 2024

VPC run looks ok - I'm going to run one more with a build test at https://ci.adoptium.net/view/Tooling/job/VagrantPlaybookCheck/1950/

@sxa
Copy link
Member Author

sxa commented Aug 13, 2024

VPC run looks ok - I'm going to run one more with a build test at https://ci.adoptium.net/view/Tooling/job/VagrantPlaybookCheck/1950/

That ran ok but there seems to be some failures in the test section that are unrelated to this PR which I've raised at #3707

@sxa sxa merged commit 9e2354a into adoptium:master Aug 13, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ansible doc docker os:windows secure-dev Issues specific to SSDF/SLSA compliance work
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

EPIC: New Machine requirement: Windows dockerBuild containers
4 participants