Fix Hexagon build using ci.py #11304
Conversation
add post build for hexagon fix -net=host for docker
tests/scripts/ci.py
Outdated
| command.append("-t") | ||
| scripts = ["interact() {", " bash", "}", "trap interact 0", ""] + scripts | ||
|
|
||
| command.append("--net=host") |
There was a problem hiding this comment.
could we make this one an option rather than on by default? it has security implications when enabled (e.g. it grants the container processes access to all the host's network interfaces)
There was a problem hiding this comment.
it's on by default in bash.sh when used from a tty, and this doesn't run any random code just the CI scripts so I don't think exposing the host net will cause problems
There was a problem hiding this comment.
adding to @driazati's comment, this is what actually runs when we call it:
./docker/bash.sh ci_hexagon --->
docker run --workdir /home/mhessar/tvm --rm --pid=host --net=host --interactive --tty --env CI_BUILD_HOME=/home/mhessar/tvm --env CI_BUILD_USER=mhessar --env CI_BUILD_UID=1007 --env CI_BUILD_GROUP=mhessar --env CI_BUILD_GID=1007 --env CI_PYTEST_ADD_OPTIONS= --env CI_IMAGE_NAME=tlcpack/ci-hexagon:20220505-060045-500703308 --env PYTHONPATH=/home/mhessar/tvm/python --volume /home/mhessar/tvm:/home/mhessar/tvm --volume /home/mhessar/tvm/docker:/docker --volume /home/mhessar/.tvm_test_data:/home/mhessar/tvm/.tvm_test_data tlcpack/ci-hexagon:20220505-060045-500703308 bash --login /docker/with_the_same_user bash
There was a problem hiding this comment.
@Lunderberg this behavior changed to default-on in #8670. did you have rationale behind that you can remember? I feel like maybe we shouldn't do that...but if there is a common use case it breaks then it's probably not worth making folks add that option all the time.
There was a problem hiding this comment.
Hmm. My goal with #8670 was to maintain identical behavior to before the refactor. I kept the --net=host argument disable by default, but conditionally add it back in for interactive sessions whose command is bash with no additional arguments. Let me check to see if that behavior has drifted since then.
There was a problem hiding this comment.
Verifying on main, the USE_NET_HOST variable is defaulted to false, and is only enabled if the --net=host flag is passed to docker/bash.sh, or if the full command being run is bash.
There was a problem hiding this comment.
ok, so i think the behavior here should mimic the behavior you're describing.
There was a problem hiding this comment.
can we merge it then after I resolve the conflict?
There was a problem hiding this comment.
so based on @Lunderberg 's comment i think we should only add --net=host when:
- the command line being run is only
bash - the user explicitly supplied
--net=hosttodocker/bash.sh
i think we should adopt that in this PR, then we can merge. thoughts?
There was a problem hiding this comment.
I removed --net for now. PTAL!
4 participants
cc @driazati @areusch