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

feat(installer): add windows installer script #1503

Merged
merged 5 commits into from Jan 4, 2024
Merged

feat(installer): add windows installer script #1503

merged 5 commits into from Jan 4, 2024

Conversation

supleed2
Copy link
Contributor

Description of change

Adds Powershell script, following steps as in bash install script, closes #1496.

Install command added to readme will work once shuttle-hq/www#220 is merged.

How has this been tested? (if applicable)

Tested locally on multiple machines, including commenting out codepaths to check both sides of branches. The section for downloading a binary may need to be updated in the future if CI is updated to produce aarch64 windows binaries.

Copy link
Member

@jonaro00 jonaro00 left a comment

Choose a reason for hiding this comment

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

Looks good. Will test on my windows machine later

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@jonaro00
Copy link
Member

jonaro00 commented Jan 2, 2024

Tried it on a machine that has tar but not rust:

  • Script assumes Home.cargo\bin exists, and fails if it doesn't (which is not a blocker; we can add checking for sufficient rust install to both scripts later)

(added the bin folder manually, then)

  • The exit call after successful install made my powershell window close itself, which is probably not desired. (is it due to not running in a subshell anymore?)

(opened and ran it again)

  • The raw download route fails if the destination file already exists. It should overwrite an existing destiantion.

  • Other caveat: Only if on non-AMD64 and with no tar.exe does it offer to install rust 🤔 . As said above, we will probably revisit this for both scripts so that the rust check+install can be the first step for both.

install.ps1 Outdated Show resolved Hide resolved
@supleed2
Copy link
Contributor Author

supleed2 commented Jan 2, 2024

  1. Maybe the binary download section can go before cargo.exe install cargo-shuttle --locked inside the cargo.exe check, since this would indicate cargo_home should exist?

Should the script create the bin folder in cargo_home if it doesn't exist?

  1. Yes, running in the same shell means it exits, 2 options, prompt user to press enter before exiting, or nest the code in if-else branches and remove all exit calls?

  2. I can just add -Force to the Move-Item call?

@jonaro00
Copy link
Member

jonaro00 commented Jan 3, 2024

  1. Not sure what you mean by that. The binary download is higher prio than cargo install. I don't think we should create "our own" cargo bin folder, but instead let the check (in later PR) for that cargo is installed guard against this error.
  2. Hmm, ok. I guess that is due to iex not spawning a sub-process for evaluating it. Maybe it is possible to wrap it in a function and use Return instead? This isn't a major blocker, so it can be left to fix in an upcoming PR.
  3. Sounds good.

After resolving (3), this is probably ready to be merged.

@supleed2
Copy link
Contributor Author

supleed2 commented Jan 4, 2024

Added the -Force flag and wrapped it in a function, it also restores the $ErrorActionPreference since it no longer exits.

@jonaro00 jonaro00 requested a review from oddgrd January 4, 2024 14:37
Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

Awesome, thanks so much @supleed2!

@jonaro00 jonaro00 merged commit 52ca24a into shuttle-hq:main Jan 4, 2024
35 checks passed
oddgrd pushed a commit that referenced this pull request Jan 8, 2024
* feat(installer): add windows installer script

* fix: Update command to use pipe

* Make confirm prompt case-insensitive

* Added -Force flag to Move-Item

* Replace exit with return and wrap in function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Windows Powershell install script
3 participants