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

Partially revert commit that broke using emrun.py outside the tree #13412

Merged
merged 4 commits into from Mar 16, 2021

Conversation

juj
Copy link
Collaborator

@juj juj commented Feb 4, 2021

Partially revert commit that broke using emrun.py outside the tree. People often deploy emrun.py into their own CIs and infrastructure outside Emscripten.

commit 4fc6505
Author: Sam Clegg sbc@chromium.org
Date: Sun Sep 13 19:05:11 2020 -0700

Use shared wrappers for shutils. NFC. (#12200)

We we not being consistent in our use of safe_copy and
safe_move and try_delete.

@juj juj force-pushed the fix_emrun branch 2 times, most recently from 53d9861 to 1316d8d Compare February 13, 2021 07:03
@juj
Copy link
Collaborator Author

juj commented Feb 13, 2021

Also fixed emrun to be python 2 compatible (not sure when it lost that support).

emrun.py Outdated Show resolved Hide resolved
emrun.py Show resolved Hide resolved
@sbc100
Copy link
Collaborator

sbc100 commented Feb 15, 2021

Sorry I was unaware that we want to be able run emrun outside of the emscripten tree or that we needed to be able to run it under python 2.

We should add some tests for both of these requirements if we want to keep them. In particular, I wonder if we can drop the python2 requirement since it makes testing harder. The out-of-tree requirement seems easy enough to test, but what is the use case exactly?

@juj
Copy link
Collaborator Author

juj commented Feb 16, 2021

I don't know if it is viable to add python2 testing for this, since we don't depend on it anymore.

The use case is that people download this tool separately out of Emsdk/Emscripten repo, to use it locally to run web servers on their system. They may not have python 3, running into issues.

@sbc100
Copy link
Collaborator

sbc100 commented Feb 16, 2021

I don't know if it is viable to add python2 testing for this, since we don't depend on it anymore.

The use case is that people download this tool separately out of Emsdk/Emscripten repo, to use it locally to run web servers on their system. They may not have python 3, running into issues.

But do we have users that do that? This is the first time I'm hearing about it. I don't mind doing extra work here if folks really use it this way, I just want to check it worth our time.

sbc100 added a commit that referenced this pull request Feb 24, 2021
sbc100 added a commit that referenced this pull request Feb 24, 2021
@sbc100 sbc100 mentioned this pull request Feb 24, 2021
sbc100 added a commit that referenced this pull request Feb 24, 2021
sbc100 added a commit that referenced this pull request Feb 24, 2021
Base automatically changed from master to main March 8, 2021 23:49
juj added 4 commits March 16, 2021 17:34
commit 4fc6505
Author: Sam Clegg <sbc@chromium.org>
Date:   Sun Sep 13 19:05:11 2020 -0700

    Use shared wrappers for shutils. NFC. (emscripten-core#12200)

    We we not being consistent in our use of safe_copy and
    safe_move and try_delete.
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm

However, we really should add tests if we want to support:

a) running emrun out-of-tree
b) running emrun run out-of-tree with only python2 available

Are we sure we have users that need both of these?

If so, and we want to continue to support these users, we should really have tests for both these configurations.

@kripken do you know of any such users?

@juj juj merged commit 4778850 into emscripten-core:main Mar 16, 2021
@juj
Copy link
Collaborator Author

juj commented Mar 16, 2021

The source of these python 2 users come from Unity users who are looking for an ad hoc server to use. Some of those have had just python2 installed (presumably on macOS with no python3 installation).

I am fine keeping python 2 weakly supported for this script. There are really not much active changes going to it, except recent cosmetic refactoring, so I don't expect it to actively break left and right if it is not tested.

@kripken
Copy link
Member

kripken commented Mar 16, 2021

I'm not aware of additional users, but @juj's use case sounds valid to me. Testing of python2 there seems reasonable.

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.

None yet

3 participants