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

fix(exe): wasm-pack-init (1).exe should work #550

Merged
merged 1 commit into from
Feb 28, 2019
Merged

Conversation

ashleygwilliams
Copy link
Member

🔕 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?

Copy link
Contributor

@alexcrichton alexcrichton left a 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

@ashleygwilliams ashleygwilliams added needs review and removed work in progress do not merge! labels Feb 27, 2019
@ashleygwilliams
Copy link
Member Author

@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.

@alexcrichton
Copy link
Contributor

alexcrichton commented Feb 27, 2019

I, unfortunately, do not

@ashleygwilliams ashleygwilliams removed the needs tests please add tests to this PR label Feb 27, 2019
@fitzgen
Copy link
Member

fitzgen commented Feb 27, 2019

Sorry :(

@Pauan
Copy link
Contributor

Pauan commented Feb 27, 2019

@ashleygwilliams I have Windows 10, I'll try and reproduce the issue (and verify the fix).

@ashleygwilliams
Copy link
Member Author

thank you so much @Pauan i really appreciate it! i was able to reproduce the error on my machine when it was originally reported

Copy link
Contributor

@Pauan Pauan left a 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.

@ashleygwilliams
Copy link
Member Author

thank you so much @Pauan !!

@ashleygwilliams ashleygwilliams merged commit 70dba37 into master Feb 28, 2019
@ashleygwilliams ashleygwilliams deleted the install-1 branch February 28, 2019 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wasm-pack-init (1).exe does not show init instructions
4 participants