fix(build.py): git submodule update fails when installing from source distribution (sdist)
#167
Conversation
build.py
Outdated
|
|
||
| def build(): | ||
| execute("git submodule update --init --recursive", cwd=TOP_DIR) | ||
| execute("git submodule update --init --recursive || echo 'Skipping. We are installing from an sdist tarball.'", cwd=TOP_DIR) |
There was a problem hiding this comment.
What is the reason for the git submodule update --init --recursive again? If it's not necessary in build, I'm not sure why it should go in build.py, if this is for the tests or something like this, wouldn't it be better suited in whichever startup script is responsible for the tests.
There was a problem hiding this comment.
It's only used for the commonjs test suite right now.
However, setup.sh script is also executed if install from .tar.gz sdist.
Line 36 in 5b98812
There was a problem hiding this comment.
I will approve, but I think we should consider moving the submodule update to somewhere else.
I think it's important to keep in mind the semantics of tools. In this case build.py is meant to be the necessary components to build the wheel. Calling bash ./setup.sh is absolutely necessary for building the wheel as we necessarily need the spidermonkey lib. However the commonjs test suite is not necessary for building the wheel.
Ex: If it's only needed for the commonjs test suite, perhaps peter-junior can be updated to run a setup script that runs the git submodule update.
There was a problem hiding this comment.
Agree. How about moving it to tests/js/commonjs-modules.bash?
Hamada-Distributed
left a comment
Hamada-Distributed
left a comment
There was a problem hiding this comment.
Approved with minor note.
To test this, add
--no-binary pythonmonkeyflag topip installto forcibly install from source code.