Skip to content

Use a wrapper script for running emsdk.py#292

Merged
sbc100 merged 1 commit intomasterfrom
wrapper_script
Aug 27, 2019
Merged

Use a wrapper script for running emsdk.py#292
sbc100 merged 1 commit intomasterfrom
wrapper_script

Conversation

@sbc100
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 commented Jul 2, 2019

This change moves the python code for emsdk into a file ending in .py.
This script is then run via emsdk.bat on windows or emsdk (a shell
script) on non-windows.

This avoid the #!/bin/sh at the top of the python script and the "exec"
hack on the first line that re-runs it under python. Hopefully this
preserves the intent of #273 without jumping through so many hoops.

@sbc100 sbc100 requested a review from kripken July 2, 2019 22:40
@kripken
Copy link
Copy Markdown
Member

kripken commented Jul 3, 2019

cc @trzecieu

Comment thread test.py

def hack_emsdk(marker, replacement):
src = open('emsdk').read()
src = open('emsdk.py').read()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why are the tests changed? none of the old usages are tested, so we lost coverage for the old ./emsdk use pattern, which is what users are doing. best to test both (the official/intended, and also I assume some people will call the new emsdk.py directly because they find it).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This test in particular is hacking the python source itself, so it needs to change.

Comment thread test.py Outdated
check_call('python2 ./emsdk install latest-upstream')
check_call('./emsdk activate latest-upstream')
check_call('python2 ./emsdk.py install latest-upstream')
check_call('python3 ./emsdk.py activate latest-upstream')
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In addition I'd keep the old check check_call('./emsdk activate latest-upstream')

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Reverted

@trzecieu
Copy link
Copy Markdown
Collaborator

trzecieu commented Jul 3, 2019

Looks good, indeed it will be much cleaner. In the current form this script will hit issue addressed by #275. That is in case of pyenv, python3 command might be available but it will be just a mock script.

This change moves the python code for emsdk into a file ending in .py.
This script is then run via emsdk.bat on windows or emsdk (a shell
script) on non-windows.

This avoid the #!/bin/sh at the top of the python script and the "exec"
hack on the first line that re-runs it under python.  Hopefully this
preserves the intent of #273 without jumping through so many hoops.
@sbc100 sbc100 merged commit a562ff7 into master Aug 27, 2019
@sbc100 sbc100 deleted the wrapper_script branch August 27, 2019 21:02
sbc100 added a commit that referenced this pull request Aug 29, 2019
sbc100 added a commit that referenced this pull request Aug 29, 2019
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.

3 participants