-
Notifications
You must be signed in to change notification settings - Fork 75
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
Generate windows installer. #1726
Conversation
c08caac
to
bc6e5a0
Compare
bc6e5a0
to
20fb89a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice 👏🏻. One ask otherwise lgtm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three more questions.
Did the design folks provide the ico? Looks like the stellar.ico is a white on black icon. It's more common for the black on white icon to be used, so we should probably use it, or check in with the design folks.
The EnVar plugin is embedded as a binary and binaries are always a risk. Given the cli handles keys, we should be particularly careful about embedding the binary especially if we haven't built it from source ourselves. Even if we have built it ourselves isn't great. What do we need EnVar for? Can we get away without it?
The license for EnVar looks custom, or maybe based off the zlib/libpng licenses. The license looks straightforward to me personally, but we need the legal team to approve it before embedding.
Didn't run this by them, but I've switched to white bg with black foreground. The icon's quality is super low, so it doesn't really matter (without it, it's just a blue square).
We can't (or at least, shouldn't). That's the recommended way. Trying to manually handle the PATH can lead to the path being truncated/corrupted (as stated in their docs).
I'll run this by them. |
cec03be
to
39973ec
Compare
@leighmcculloch this is good to go! Just tested this installer and it worked! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion, regardless lgtm. 👏🏻
The previous approach can lead to truncated/corrupted Path. https://nsis.sourceforge.io/Setting_Environment_Variables
f9c88dc
to
a89706f
Compare
What
Create installer for Windows.
Why
This is a first step on making stellar-cli available on winget.
Known limitations
N/A