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

Adding a PowerShell installation script #433

Open
AntoniosBarotsis opened this issue Jun 13, 2022 · 5 comments
Open

Adding a PowerShell installation script #433

AntoniosBarotsis opened this issue Jun 13, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@AntoniosBarotsis
Copy link

Description

Adding a PowerShell installation script would make installation easier for windows users over the current very manual installation.

I have some experience with PowerShell but I have never written something like this before. I've made a first draft so if this issue gets a green flag I can start working on a pull request however I would really appreciate it if someone more experienced than me in PowerShell would review this :)

@AntoniosBarotsis AntoniosBarotsis added the enhancement New feature or request label Jun 13, 2022
@stepchowfun
Copy link
Owner

Hi @AntoniosBarotsis, thanks for filing this issue! This seems like it would be a nice thing to have.

Unfortunately I don't have much PowerShell experience, and most of my Windows knowledge is outdated by about a decade. So I probably wouldn't feel very confident reviewing a PR for this. I don't run Windows, except in rare situations such as when I was writing the (manual, as you noted) Windows installation instructions in the first place.

I wish we had some Windows experts who we could consult for this, but for now unfortunately I'm the bottleneck here. So I don't want to make any promises regarding my ability to accept a pull request here.

I'm happy to discuss solutions here and even take a look at any pull requests, but I don't want to get your hopes up. There's a good chance I will just be too out of my depth and will be unable to move forward on it.

@AntoniosBarotsis
Copy link
Author

Thanks for the reply! Just pushed the file. Honestly, if you've ever written PowerShell you should be able to read the important parts of it so that should be fine, nothing complicated is going on in the file. I was more concerned about potential bad practices I may be using.

I have run it locally (although incrementally as I was finishing it up) and it seems to be working fine. It will always download the latest available version which might be something you want changed although from what I see that is what your install.sh does as well.

Let me know if you have any questions

@stepchowfun
Copy link
Owner

Honestly, if you've ever written PowerShell you should be able to read the important parts of it so that should be fine

Well, as I mentioned, I don't really know PowerShell. I know that might be a frustrating reason for my reluctance about this change, but it's the truth. So I can't reach the same conclusion that it "should be fine"—even if it probably is.

I have questions about almost every line in the script. For example, when I see set-location $PSScriptRoot, I'm immediately wondering if $PSScriptRoot should be quoted to handle spaces (as would be needed in Bash). I think the answer is "no", but that's only after a few minutes of Googling around (and I'm still not 100% sure). Furthermore, I don't know the implications of UAC, and if that gives the whole script administrator permissions, that seems like too much to me. Those are some examples of questions that I have, but listing all my questions would be an exercise in itself.

So basically it would take me some effort to review this PR, and that effort would be essentially equivalent to just figuring it out myself (e.g., exploring all the command-line flags to make sure they're doing what we want). I find that this is often the case with pull requests, unless they are simple changes like small bugfixes, typo fixes, etc. Also the README would need to be updated to tell users how to use this script (is it even simpler than just downloading the binary from the releases page and adding it to your PATH?), we should add a line break at the end of the file for consistency with every other file in the repo, etc. Also while the script does match the behavior of the Linux/macOS one in that it downloads the latest version, it doesn't have feature parity with the existing installer script (e.g., the ability to explicitly pin to a specific version which is useful for reproducibility).

The unfortunate reality is that, while I think this might be a good contribution, I don't think I'm up for putting in the work to validate it and playing ping pong with you to implement whatever nitpicky changes I ask for. I know this is disappointing, and I hope my first message above set expectations appropriately. I hope I haven't wasted too much of your time.

@AntoniosBarotsis
Copy link
Author

AntoniosBarotsis commented Jun 14, 2022

That's understandable, I'll just reply to the points you made in case this gets reviewed sometime in the future.

  • The set-location only matters for the initial download location of the .exe file; I used that to make sure that the download directory is valid instead of downloading at whatever the default one would be or trying to guess if there's a Downloads directory
  • The admin perms are needed to move the file over to the program files (and I'm assuming to also add it to the path)
  • Running the script seems simpler to me as you would only need to run the following:
    Invoke-RestMethod https://raw.githubusercontent.com/AntoniosBarotsis/toast/main/install.ps1 | Invoke-Expression
    As I'm writing this I realized that perhaps users would need to have a specific version of PowerShell for this to work (there are some differences between Windows PowerShell and PowerShell core and I'm not sure if I'm using something that is specific to core here). By default, users do not have Core installed.
  • I did notice something about a specific version in your install.sh file but wasn't sure what it was for, makes sense now I guess. This is definitely not hard to add but since this isn't going anywhere I'll skip over it

Honestly, if this (understandably) never gets merged I still had my bit of fun writing it and learned a few things on the way. Thanks for the replies!

If I may add to this, there are a few package managers for windows such as Chocolatey and Scoop (which I haven't used so not sure how that works). They handle paths and installations for you if you have the patience to decipher their docs. I am pretty sure that you could make a package that is just the exe file.

@stepchowfun
Copy link
Owner

Honestly, if this (understandably) never gets merged I still had my bit of fun writing it and learned a few things on the way.

😄

If I may add to this, there are a few package managers for windows such as Chocolatey and Scoop (which I haven't used so not sure how that works).

Yeah, I think that's another part of my hesitation here. I don't really know what Windows users expect when installing software. E.g., what is the most popular way to install software on Windows? Is creating a PowerShell script the right way to go?

I think I'd need to understand the landscape better before investing more in any particular solution (beyond just asking users to just download the binary manually).

I know this might not be the best outcome for your change, but I hope this gives you and any passersby a bit of confidence in Toast after seeing how persnickety I am about what goes into it and my requirement that I can vet everything that the code is doing. And now everyone knows how ignorant I am about Windows, too. 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants