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.

Sign up for GitHub

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

Jump to bottom

Support building Windows msi file #3999

Merged
merged 2 commits into from
Sep 12, 2019
Merged

Support building Windows msi file #3999

merged 2 commits into from
Sep 12, 2019

Conversation

jwhonce
Copy link
Member

@jwhonce jwhonce commented Sep 11, 2019

  • Update Makefile to build msi
  • Add podman.wxs to define podman.msi
    • Version information provided by Makefile
  • Add podman.bat wrapper for podman-remote-windows.exe to ensure environment
  • Add wix xml schemas for reference

Signed-off-by: Jhon Honce [email protected]

@jwhonce jwhonce requested review from lsm5, cevich and baude September 11, 2019 21:37
@jwhonce jwhonce self-assigned this Sep 11, 2019
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwhonce

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-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 11, 2019
@mheon
Copy link
Member

mheon commented Sep 11, 2019

13000 line XML file... oh dear

@jwhonce
Copy link
Member Author

jwhonce commented Sep 11, 2019

13000 line XML file... oh dear

Be afraid... :-)

@jwhonce jwhonce closed this Sep 11, 2019
@jwhonce jwhonce reopened this Sep 11, 2019
@jwhonce
Support building Windows msi file
ecb958e 
* Update Makefile to build msi
* Add podman.wxs to define podman.msi
  * Version information provided by Makefile
* Add podman.bat wrapper for podman-remote-windows.exe to ensure environment
* Add wix xml schemas for reference

Signed-off-by: Jhon Honce <[email protected]>
@openshift-ci-robot openshift-ci-robot added size/M and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 11, 2019
@rhatdan
Copy link
Member

rhatdan commented Sep 12, 2019

Do we end up with man pages or Windows equivalents?

@TomSweeneyRedHat
Copy link
Member

I'm not a Windows heavy, but LGTM and happy green tests.

mheon
mheon reviewed Sep 12, 2019
View reviewed changes
Makefile Outdated
@@ -164,6 +165,10 @@ podman: .gopathok $(PODMAN_VARLINK_DEPENDENCIES) ## Build with podman
podman-remote: .gopathok $(PODMAN_VARLINK_DEPENDENCIES) ## Build with podman on remote environment
$(GO_BUILD) $(BUILDFLAGS) -gcflags '$(GCFLAGS)' -asmflags '$(ASMFLAGS)' -ldflags '$(LDFLAGS_PODMAN)' -tags "$(BUILDTAGS) remoteclient" -o bin/$@ $(PROJECT)/cmd/podman

.PHONY: podman.msi
podman.msi: podman-remote-windows ## Will always rebuild exe as there is no podman-remote-windows.exe target to verify timestamp
wixl -D VERSION=$(RELEASE_NUMERIC) -o bin/podman-$(RELEASE_NUMBER).msi contrib/msi/podman.wxs
Copy link
Member

Choose a reason for hiding this comment

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

Can we run this as part of the Windows cross-build Cirrus task?

Copy link
Member

Choose a reason for hiding this comment

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

yes we should.

Copy link
Member

Choose a reason for hiding this comment

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

contrib/cirrus/build_release.sh is the place for that to happen (there's already a case-section for it).

FYI: I started the needed updates to contrib/cirrus/upload_release_archive.sh so the file can be updloaded to the place where users can download it.

cevich
cevich reviewed Sep 12, 2019
View reviewed changes
Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

Except for my OCD-twitch over so many RELEASE_* LGTM. Thanks Jhon.

Makefile Outdated
@@ -164,6 +165,10 @@ podman: .gopathok $(PODMAN_VARLINK_DEPENDENCIES) ## Build with podman
podman-remote: .gopathok $(PODMAN_VARLINK_DEPENDENCIES) ## Build with podman on remote environment
$(GO_BUILD) $(BUILDFLAGS) -gcflags '$(GCFLAGS)' -asmflags '$(ASMFLAGS)' -ldflags '$(LDFLAGS_PODMAN)' -tags "$(BUILDTAGS) remoteclient" -o bin/$@ $(PROJECT)/cmd/podman

.PHONY: podman.msi
podman.msi: podman-remote-windows ## Will always rebuild exe as there is no podman-remote-windows.exe target to verify timestamp
wixl -D VERSION=$(RELEASE_NUMERIC) -o bin/podman-$(RELEASE_NUMBER).msi contrib/msi/podman.wxs
Copy link
Member

Choose a reason for hiding this comment

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

yes we should.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated
@@ -164,6 +165,10 @@ podman: .gopathok $(PODMAN_VARLINK_DEPENDENCIES) ## Build with podman
podman-remote: .gopathok $(PODMAN_VARLINK_DEPENDENCIES) ## Build with podman on remote environment
$(GO_BUILD) $(BUILDFLAGS) -gcflags '$(GCFLAGS)' -asmflags '$(ASMFLAGS)' -ldflags '$(LDFLAGS_PODMAN)' -tags "$(BUILDTAGS) remoteclient" -o bin/$@ $(PROJECT)/cmd/podman

.PHONY: podman.msi
podman.msi: podman-remote-windows ## Will always rebuild exe as there is no podman-remote-windows.exe target to verify timestamp
wixl -D VERSION=$(RELEASE_NUMERIC) -o bin/podman-$(RELEASE_NUMBER).msi contrib/msi/podman.wxs
Copy link
Member

Choose a reason for hiding this comment

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

contrib/cirrus/build_release.sh is the place for that to happen (there's already a case-section for it).

FYI: I started the needed updates to contrib/cirrus/upload_release_archive.sh so the file can be updloaded to the place where users can download it.

@jwhonce
Add podman icon to installer
046c081 
Update Makefile per review comments

Signed-off-by: Jhon Honce <[email protected]>
@jwhonce
Copy link
Member Author

jwhonce commented Sep 12, 2019

Do we end up with man pages or Windows equivalents?

Coming in follow-on card. Need to research markdown to CHM converters to automate process.

@rhatdan
Copy link
Member

rhatdan commented Sep 12, 2019

/lgtm
/hold
Run hold cancel when this passes tests.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2019
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2019
@jwhonce
Copy link
Member Author

jwhonce commented Sep 12, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2019
@openshift-merge-robot openshift-merge-robot merged commit af8fedc into containers:master Sep 12, 2019
codefromthecrypt
codefromthecrypt reviewed Jul 30, 2021
View reviewed changes
contrib/msi/podman.wxs

<InstallExecuteSequence>
<RemoveExistingProducts Before="InstallInitialize"/>
<Custom Action="ChangePath" After="InstallServices">NOT Installed</Custom>
Copy link

Choose a reason for hiding this comment

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

I noticed that this doesn't cleanup the PATH on uninstall. Seems a bit dodgy to try to do that actually, but mentioning in case you know a clean way.

Copy link
Member

Choose a reason for hiding this comment

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

Can you open a fresh issue about this? We don't actively review two-year-old closed PRs so this will get lost otherwise

Copy link

Choose a reason for hiding this comment

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

np thanks for offering it issue worthy! #11089

@codefromthecrypt codefromthecrypt mentioned this pull request Aug 1, 2021
Closed
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mheon mheon mheon left review comments

@cevich cevich cevich left review comments

@codefromthecrypt codefromthecrypt codefromthecrypt left review comments

@lsm5 lsm5 Awaiting requested review from lsm5

@baude baude Awaiting requested review from baude

@vrothberg vrothberg Awaiting requested review from vrothberg

Assignees

@jwhonce jwhonce

@rhatdan rhatdan

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jwhonce @openshift-ci-robot @mheon @rhatdan @TomSweeneyRedHat @codefromthecrypt @cevich @openshift-merge-robot