Add windows docker build script

[youngbash88]

Summary of the PR

• Added docker.ps1 to enable building the container using PowerShell with the following command:

  .\docker.ps1 build

  And publishing the container using:

  .\docker.ps1 publish

as in docker.sh to resolve #49 (Note: didn't update the manifest in docker.sh)

• Modified .github/workflows/docker-publish.yml to: Add build-windows job for Windows containers.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

• All commits in this PR have Signed-Off-By trailers (with
  git commit -s), and the commit message has max 60 characters for the
  summary and max 75 characters for each description line.
• All added/changed functionality has a corresponding unit/integration
  test.
• All added/changed public-facing functionality has entries in the "Upcoming
  Release" section of CHANGELOG.md (if no such section exists, please create one).
• Any newly added unsafe code is properly documented.

[stefano-garzarella]

Please, can you update README.md too?

[stefano-garzarella]

@youngbash88 thanks for the PR, I just enabled the CI, but it's failing soon, can you take a look?

[roypat]

Hey, I'm not really familiar with the context behind #49, could you provide a bit more motivation for this? Will it help with testing some of the mshv stuff in GHA? Because our buildkite CI infra doesnt have any windows hosts, so a windows docker container can't run anywhere in CI that I'm aware of.

[youngbash88]

Hey, I'm not really familiar with the context behind #49, could you provide a bit more motivation for this?

Hey, thanks for the clarification! The motivation behind #49 was to add a script similar to docker.sh for Windows. I initially modified the CI to add build-windows, thinking it could be useful for future testing.

Because our buildkite CI infra doesnt have any windows hosts, so a windows docker container can't run anywhere in CI that I'm aware of.

Since the Buildkite CI infra doesn’t have Windows hosts, the Windows build won’t work in the current setup. I’ll go ahead and remove the Windows build step from the CI configuration. Should I open an issue to track this for potential Windows host support in the future?

[stefano-garzarella]

@youngbash88 there is something wrong with the rebase here, can you do it properly?

[youngbash88]

@stefano-garzarella done.

[stefano-garzarella]

@youngbash88 there is still a commit with just "test"

[youngbash88]

@stefano-garzarella I squashed the commits and updated the commit message. LMK if it's good or if any changes are needed

[stefano-garzarella]

I left minor comments, but I didn't test the windows script since I don't have access to windows machine. The changes to linux scripts LGTM!

[RuoqingHe]

I think we can ask people in mshv to get involved on this topic, IMO they most likely need this

[stefano-garzarella]

I think we can ask people in mshv to get involved on this topic, IMO they most likely need this

Good point, let me ping the code owners of that crate.

@russell-islam @liuw @NunoDasNeves @jinankjain @praveen-pk please, can you take a look?

[liuw]

I think we can ask people in mshv to get involved on this topic, IMO they most likely need this

Good point, let me ping the code owners of that crate.

@russell-islam @liuw @NunoDasNeves @jinankjain @praveen-pk please, can you take a look?

We use Linux for our infrastructure through and through. I myself have close to zero knowledge on PowerShell. :-)

[russell-islam]

I think we can ask people in mshv to get involved on this topic, IMO they most likely need this

Good point, let me ping the code owners of that crate.
@russell-islam @liuw @NunoDasNeves @jinankjain @praveen-pk please, can you take a look?

We use Linux for our infrastructure through and through. I myself have close to zero knowledge on PowerShell. :-)

+1

[stefano-garzarella]

@youngbash88 LGTM, but please organize the commits better:

• avoid doing multiple stuff in a single commit (e.g. last commit can be splitted at least in 2 commits)
• avoid changing code introduced by the PR in next commits (e.g. in this case, introduce the docker.env and changes related to linux first, then introduce the docker.ps1, so you can use that file directly
• avoid merging commit from main, just rebase this PR on top of it

[stefano-garzarella]

IIUC it looks like there is some issues with CI and RUST_TOOLCHAIN changes.
@youngbash88 can you take a look?

[youngbash88]

@stefano-garzarella It supposed to work now. can you activate the CI?

[stefano-garzarella]

@youngbash88 please check patch 3 description, try to keep lines around 75 columns.

It looks like CI is still failing.

[youngbash88]

@youngbash88 please check patch 3 description, try to keep lines around 75 columns.

@stefano-garzarella I updated this commit message, is that what you meant?
Also i changed the approach, now build_container.sh source rust toolchain from docker.env directly

[stefano-garzarella]

Please try to write better commit. I suggest taking a look at https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

For example Modify docker-publish.yml is not a good title, better something like:

Add docker.env to workflow paths

build_container.sh sources RUST_TOOLCHAIN from it, so we need to ...

So summarize in the title what the patch is doing, then you can add details in the body of the commit message, which provides more context and also the reason (like you did).

That said, I think that commit can be removed if we implement what I suggested, like we do for other env vars.

[youngbash88]

@stefano-garzarella can you please enable the CI?

[stefano-garzarella]

I don't want to be picky, but please check commit messages. Use imperative (not past verb), also explain reason, so in the future we can remember what we did (and why we did).

Patch 1:

Add docker.env

We are going to add a windows script, so concentrate the environment
variables in one file to use them in the linux and windows script.

Patch 2:

Define RUST_TOOLCHAIN in docker.env

Instead of defining RUST_TOOLCHAIN in both Dockerfile and
Dockerfile.windows.x86_64files, let's define it in one place
(docker.env and pass it as an environment variable on both
docker.sh and github workflows.

etc.

[stefano-garzarella]

@youngbash88 thanks, LGTM!

[youngbash88]

@stefano-garzarella Thanks for your time!

[stefano-garzarella]

@RuoqingHe @roypat PTAL.

I think for now we can just merge it also if our CI doesn't support it yet, allowing people to build our container on windows as well.

[RuoqingHe]

Great work - could @youngbash88 provide some screenshots showing this actually works for testing rust-vmm crates on windows, and some OS details as well, that would be very helpful 🙂

[youngbash88]

@RuoqingHe

[RuoqingHe]

Looks good to me, thanks @youngbash88 ❤️