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

cleanup PATH on MSI uninstall #11089

Closed
codefromthecrypt opened this issue Aug 1, 2021 · 30 comments · Fixed by #12597
Closed

cleanup PATH on MSI uninstall #11089

codefromthecrypt opened this issue Aug 1, 2021 · 30 comments · Fixed by #12597
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. volunteers-wanted Issues good for community/volunteer contributions

Comments

@codefromthecrypt
Copy link

Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)

/kind bug

Description

Right now, podman is added to the PATH in windows on install, but this is left after uninstall

Steps to reproduce the issue:

  1. Install the MSI

  2. Uninstall it

  3. Look at the PATH

Describe the results you received:
the PATH still includes the INSTALLDIR

Describe the results you expected:
the PATH should remove the INSTALLDIR on uninstall, even if the PATH was mutated by something else between install and uninstall

Additional information you deem important (e.g. issue happens only occasionally):
lines https://github.com/containers/podman/blob/main/contrib/msi/podman.wxs#L34-L35

This has been the case since initial support and so far no one complained
#3999 (comment)

** Epilogue on another way **
The windows-only WIX toolset treats environment as a first class feature. For example, there is documentation on how to add and remove the INSTALLDIR here:
https://www.firegiant.com/wix/tutorial/com-expression-syntax-miscellanea/environmentally-friendly/

As far as I can tell, this project sets the floor at wix features supported by GNOME msitools. This means it works equally on nix and windows, except there is a lowest common denominator concern. FWI I agree with optimizing for GNOME, though it runs into this.

Another way out would be to help GNOME msitools implement the same Environment component element and then do it right. I don't have the chops to do this quickly, but would help review etc any solution including and ideally this!

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 1, 2021
@codefromthecrypt
Copy link
Author

looks like the Environment node is tracked here.. I guess we can help that or until that we can consider a setx revert

@codefromthecrypt
Copy link
Author

actually looks very close to merge! https://gitlab.gnome.org/GNOME/msitools/-/merge_requests/42

@Luap99
Copy link
Member

Luap99 commented Aug 2, 2021

Feel free to open a PR if you know how to revert this with setx on uninstall. Otherwise we are blocked until Environment is supported by wixl.

@codefromthecrypt
Copy link
Author

having failed at the former with a very slow windows laptop, I'll go for the latter!

@cevich
Copy link
Member

cevich commented Aug 3, 2021

Will the "new way" break in the older tooling?

Reason I ask is, we test the windows release build in CI...which has tooling bundled in at VM Image build time. In other words (if it's not backwards compatible), we may need to coordinate a XML change with updated VM images (with new tooling). I can help with that part, once the tooling is installable in Fedora.

@codefromthecrypt
Copy link
Author

well the feature isn't new to windows, just the portability wasn't added to GNOME msitools.

So whatever runs the commands here to generate the MSI will need to have the latest version of msitools once that's updated, synchronously or ahead of merging here.

I doubt it would work with old versions as msitools tends to crash on anything it doesn't expect.

@baude
Copy link
Member

baude commented Aug 4, 2021

once that is updated == gnome msitools?

@cevich
Copy link
Member

cevich commented Aug 4, 2021

Ya. So someone gives me a shout when that version is in the latest Fedora, I'll build new VM images to grab it. Then the image ID change can go in along with a podman PR that updates the XML. (At least that's my understanding of the proposal).

@codefromthecrypt
Copy link
Author

fyi I raised this issue for msitools being the default for GHA although I'm not sure it impacts you. If it did, basically the images would need to be upgraded once the Environment feature lands https://gitlab.gnome.org/GNOME/msitools/-/issues/41

@github-actions
Copy link

github-actions bot commented Sep 4, 2021

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Sep 7, 2021

@codefromthecrypt @cevich what is the state of this issue?

@cevich
Copy link
Member

cevich commented Sep 7, 2021

I don't see any activity upstream, but maybe I'm not looking at the right thing.

@codefromthecrypt
Copy link
Author

https://gitlab.gnome.org/GNOME/msitools/-/merge_requests/42 was pinged last week but nothing I can see happened, yet. maybe close this as unsolvable if the project has stalled out and can't complete it by the end of the month?

@cevich
Copy link
Member

cevich commented Sep 8, 2021

SGTM

@github-actions
Copy link

github-actions bot commented Oct 9, 2021

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Oct 12, 2021

@codefromthecrypt Any movement?

@codefromthecrypt
Copy link
Author

codefromthecrypt commented Oct 12, 2021 via email

@max06
Copy link

max06 commented Nov 9, 2021

Jumping in here: Did something happen? Is switching to another installer an option?

@rhatdan
Copy link
Member

rhatdan commented Nov 9, 2021

I would be fine with that. (I know squat about Windows. Last touched in 1998)

@cevich
Copy link
Member

cevich commented Nov 9, 2021

Jumping in here: Did something happen? Is switching to another installer an option?

I don't see any movement in the upstream PRs linked above.

I'm happy to help with switching to another tool/installer but....

I know squat about Windows. Last touched in 1998

Funny, that's the same year I gave up on it 😆

Seriously though...we do have the possibility of running the binary and installer build on a Windows Container (docs), if that's "easier" for someone (I just have no idea how).

@max06
Copy link

max06 commented Nov 9, 2021

Well, I'm a windows user and admin, but I never did packaging for it. The easiest way (without building actual installers) would probably be using chocolatey or win-get. Scripting instead of building. But I've never tried.

Happy to build whatever you need in docker... 😂

@codefromthecrypt
Copy link
Author

The good news is the current problem is limited in scope to a point few would notice (ex who would look at the PATH after uninstall and notice aren't that many). The other good news is the upstream project isn't dead https://gitlab.gnome.org/GNOME/msitools/-/merge_requests?scope=all&state=merged

The reality I think is someone needs to recreate the linked PR as the author went AWOL. I think if the same work had a living person on it, the project would merge it. That the author dropped it left things in a zombie state unnecessarily IOTW I'll ask for a volunteer to redo this, and if no one replies I can try to learn the toolchain to do it myself, but hoping someone else is more ready https://gitlab.gnome.org/GNOME/msitools/-/merge_requests/42

@max06
Copy link

max06 commented Nov 10, 2021

Sadly it also seems to affect #11416 - there's a single double-quote at the end of the path after installation.

@codefromthecrypt
Copy link
Author

codefromthecrypt commented Nov 10, 2021 via email

@cevich
Copy link
Member

cevich commented Nov 10, 2021

Well, I'm a windows user and admin, but I never did packaging for it.

If you speak batch/powershell, you're well ahead of basically everybody here. You're more than welcome to experiment/learn in a PR. Something like...

diff --git a/.cirrus.yml b/.cirrus.yml
index 9897a9f7f..a59c6893d 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -302,8 +302,6 @@ alt_build_task:
     matrix:
       - env:
             ALT_NAME: 'Build Each Commit'
-      - env:
-            ALT_NAME: 'Windows Cross'
       - env:
             ALT_NAME: 'Build Without CGO'
       - env:
@@ -371,6 +369,28 @@ osx_alt_build_task:
     always: *binary_artifacts


+# Confirm building the remote client, natively in a Windows container.
+windows_alt_build_task:
+    name: "Windows Cross"
+    alias: windows_alt_build
+    depends_on:
+        - build
+    env:
+        <<: *stdenvars
+        # Windows platform variation prevents this being included in alt_build_task
+        TEST_FLAVOR: "altbuild"
+        ALT_NAME: 'Windows Cross'
+    windows_container:
+        image: 'cirrusci/windowsservercore:2019'
+    setup_script:
+        - choco install -y ...
+        - ...stuff...
+        - go version
+    build_script:
+        - make podman-remote-release-windows_amd64.zip
+    always: *binary_artifacts
+
+
 # Verify podman is compatible with the docker python-module.
 docker-py_test_task:
     name: Docker-py Compat.

(Note: This assumes the proper Makefile changes - or just replace the call to make with whatever build commands are needed)

@max06
Copy link

max06 commented Nov 10, 2021

Oh ... I tricked myself into that one, didn't I? 😅

Not promising anything here, I have no clue about your toolchain at all, or about cirrus. But I'll see what I can figure out!

@cevich
Copy link
Member

cevich commented Nov 10, 2021

Yep, and I jumped on the opportunity 😁 But we can offer you plenty of 🍪 in exchange for your toil 🤣

No worries in any case, what we have now mostly "works", so it's fine if it's only a learning exercise, or you abandon it from the start in favor of more healthy 🥕

@Luap99
Copy link
Member

Luap99 commented Nov 10, 2021

If we use windows we can use wix to create the msi installer, this would allow us to use all msi features instead of being limited to msitools on linux.

@max06
Copy link

max06 commented Nov 10, 2021

There's actually a docker image for wix on linux.

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan rhatdan added volunteers-wanted Issues good for community/volunteer contributions and removed stale-issue labels Dec 13, 2021
n1hility added a commit to n1hility/podman that referenced this issue Dec 16, 2021
Fixes containers#11089 - cleanup PATH on MSI uninstall
Additionally fixes scenarios where the path can be overwritten by setx
Also removes the console flash, since the helper is built as a silent gui
Helper executable can be rerun by user to repair PATHs broken by other tools
Utilizes executable location instead of passed parameters to remove delicate escaping requirements

[NO NEW TESTS NEEDED]

Signed-off-by: Jason T. Greene <[email protected]>
n1hility added a commit to n1hility/podman that referenced this issue Dec 23, 2021
Fixes containers#11089 - cleanup PATH on MSI uninstall
Additionally fixes scenarios where the path can be overwritten by setx
Also removes the console flash, since the helper is built as a silent gui
Helper executable can be rerun by user to repair PATHs broken by other tools
Utilizes executable location instead of passed parameters to remove delicate escaping requirements

[NO NEW TESTS NEEDED]

Signed-off-by: Jason T. Greene <[email protected]>
openshift-merge-robot added a commit that referenced this issue Dec 23, 2021
[Fixes #11089] Switch to a new installer approach using a path manipulation helper
@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 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. volunteers-wanted Issues good for community/volunteer contributions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants