-
Notifications
You must be signed in to change notification settings - Fork 409
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
fix(exe): wasm-pack-init (1).exe should work #550
Conversation
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.
This looks fine by me, I think it's not so much security implications but moreso just making sure we don't accidentally try to install when we shouldn't, but this should cover that.
IIRC there's not any tests already for the installer, so it's probably to just manually verify this
@fitzgen or @alexcrichton do either of you have access to a windows machine to manually test this? unfortunately mine is in Austin and i am not. |
I, unfortunately, do not |
Sorry :( |
@ashleygwilliams I have Windows 10, I'll try and reproduce the issue (and verify the fix). |
thank you so much @Pauan i really appreciate it! i was able to reproduce the error on my machine when it was originally reported |
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.
I reproduced the original bug, and also verified that this PR fixes the bug.
thank you so much @Pauan !! |
🔕 do no merge 🔕
fixes #518
@alexcrichton how should we test this? (if at all?) are there security implications for not doing the exact match that i'm not thinking of?