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:DOCS] Add doc to build podman on windows without MSYS #21910

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

l0rd
Copy link
Member

@l0rd l0rd commented Mar 1, 2024

Updated build_windows.md with a new section that documents how to build and run the windows podman client without requiring MSYS.

[NO NEW TESTS NEEDED]

Does this PR introduce a user-facing change?

None

build_windows.md Outdated Show resolved Hide resolved
build_windows.md Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
If the command returns `Restricted` you should change the ExecutionPolicy to `RemoteSigned`:

```
Set-ExecutionPolicy -ExecutionPolicy RemoteSigned -Scope CurrentUser
Copy link
Member

Choose a reason for hiding this comment

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

you prob need to note what the ramifications of this are.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean that the comment "This policy allows the execution of local PowerShell scripts" is not enough?

Copy link
Member

Choose a reason for hiding this comment

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

I think we you change something like this, it is prob a good idea to outright let people they are theoretically changing the security level of their machine; given that the default was to be tighter.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

A note saying something to that effect would be good. When ever a a change is made system wide, especially if it changes security settings, it should be highlighted.


# Downlaod gvproxy.exe and win-sshproxy.exe
# that are needed to execute the podman client
.\winmake.ps1 win-gvproxy
Copy link
Member

Choose a reason for hiding this comment

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

do you think we should have an install target in the winmake file that does this and podman and puts it into the filesystem

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. Putting it in the filesystem may be problematic. What if you already have a stable podman installed but just want to test main branch?

And I was thinking that podman.exe should look for the helpers in it's directory. This was also how I interpretated this comment. This is not working currently and if it is appropriate I can create an issue and work on it.

Another issue is that the version of gvproxy and sshproxy is currently hardcoded.

Copy link
Member

Choose a reason for hiding this comment

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

if you have a stable podman, fwiw, there will be no impact. But, what i meant is if you did something like ./winmake install then it would.


Podman requires brew -- a collection of Unix like build tools and libraries adapted for Windows. More details and
installation instructions are available from their [home page](https://www.msys2.org/). There are also premade GitHub
actions for this tool that are available.

## Install build dependencies
### Install build dependencies
Copy link
Member

Choose a reason for hiding this comment

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

this should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just this paragraph or the full "Building the Podman client and installer with MSYS2" section?

Copy link
Member

Choose a reason for hiding this comment

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

imho, msys2 is NOT required to build on windows; and adding it just confuses things. Probably others should also weigh in on this notion

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I was careful to keep both options as there was no final decision yet. But I can remove the MSYS2 instructions if that's the consensus.

build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
build_windows.md Outdated Show resolved Hide resolved
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

I hacked up a lot of the English. We don't allow pronouns (you, your, me, I, etc) in our manpages, at least we highly discourage them. I realize this is not a man page per se, but I think it reads better without the pronouns. Use the changes if you wish.

Things to note, "Podman" and not "podman" unless in a command or a command example. Watch acronyms, especially in the first instance, and in general, stay away from abbreviations. It's better to do "administrator" instead of "admin" for instance.

I did just a English/grammar check, and did not test to make sure it was technically correct.

Regardless of my red marks, a really nice addition, thanks @l0rd !

@l0rd
Copy link
Member Author

l0rd commented Mar 5, 2024

Thank you @TomSweeneyRedHat for this review. I have accepted all your suggestions.

@l0rd l0rd changed the title Add doc to build podman on windows without MSYS [CI:DOCS] Add doc to build podman on windows without MSYS Mar 5, 2024
build_windows.md Outdated

Podman on Windows can run on the [Windows Subsystem for Linux (WSL)](https://learn.microsoft.com/en-us/windows/wsl/) or
on [Hyper-V](https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/quick-start/enable-hyper-v) .
To us Hyper-V use the following command:
Copy link
Member

Choose a reason for hiding this comment

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

Suggest: To use Hyper-V, enter the foolowing command

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

build_windows.md Outdated

### Enable Hyper-V (optional)

Podman on Windows can run on the [Windows Subsystem for Linux (WSL)](https://learn.microsoft.com/en-us/windows/wsl/) or
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention that HyperV is not included by all Windows editions?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@mheon
Copy link
Member

mheon commented Mar 5, 2024

Lint complains about trailing whitespace

@l0rd l0rd force-pushed the windows-build-doc branch 2 times, most recently from a393b14 to dd1415b Compare March 8, 2024 09:47
Updated build_windows.md with a new section that
document how to build and run the windows podman
client without the need to install MSYS.

[NO NEW TESTS NEEDED]

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
@TomSweeneyRedHat
Copy link
Member

LGTM
and happy green test buttons!

@rhatdan
Copy link
Member

rhatdan commented Mar 15, 2024

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2024
Copy link
Contributor

openshift-ci bot commented Mar 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: l0rd, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit e38c713 into containers:main Mar 15, 2024
43 checks passed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jun 14, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants