You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/05/27 23:04:29 UTC

[GitHub] [tvm] mehrdadh opened a new pull request #8156: [Docker] Add environment variable for number of cores 

mehrdadh opened a new pull request #8156:
URL: https://github.com/apache/tvm/pull/8156


   This PR:
   - Adds number of core variable to docker/install/ubuntu_install_qemu script


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] tkonolige commented on a change in pull request #8156: [Docker] Add environment variable for number of cores

Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#discussion_r641037275



##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -45,7 +51,7 @@ gpg --verify qemu-5.1.0.tar.xz.sig
 tar -xf qemu-5.1.0.tar.xz
 cd qemu-5.1.0
 ./configure --target-list=aarch64-softmmu,arm-softmmu,i386-softmmu,riscv32-softmmu,riscv64-softmmu,x86_64-softmmu
-make -j2
+make -j${NUM_CORE}

Review comment:
       Why not use `nproc`?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a change in pull request #8156: [QEMU] Add number of cores, target list for build

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#discussion_r644122583



##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -45,14 +69,16 @@ p5ez/+2k4VAIwIQoP5DoO06waLBffvLIAdPPKYsx71K67OoGG2svc7duC/+5qf1x
 =hCS7
 -----END PGP ARMORED FILE-----
 EOF
-curl -OLs https://download.qemu.org/qemu-5.1.0.tar.xz
-gpg --verify qemu-5.1.0.tar.xz.sig
+    curl -OLs https://download.qemu.org/qemu-5.1.0.tar.xz
+    gpg --verify qemu-5.1.0.tar.xz.sig
+
+    tar -xf qemu-5.1.0.tar.xz
+fi
 
-tar -xf qemu-5.1.0.tar.xz
 cd qemu-5.1.0
-./configure --target-list=aarch64-softmmu,arm-softmmu,i386-softmmu,riscv32-softmmu,riscv64-softmmu,x86_64-softmmu
-make -j${NUM_CORE}
+./configure --target-list=${target_list}

Review comment:
       removed the rebuild flag.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a change in pull request #8156: [Docker] Add environment variable for number of cores

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#discussion_r641064595



##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -45,7 +51,7 @@ gpg --verify qemu-5.1.0.tar.xz.sig
 tar -xf qemu-5.1.0.tar.xz
 cd qemu-5.1.0
 ./configure --target-list=aarch64-softmmu,arm-softmmu,i386-softmmu,riscv32-softmmu,riscv64-softmmu,x86_64-softmmu
-make -j2
+make -j${NUM_CORE}

Review comment:
       Because some people are building this on a local machine to test with low number of cores and it could crash.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a change in pull request #8156: [QEMU] Add number of cores, target list for build

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#discussion_r641830114



##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -45,14 +69,16 @@ p5ez/+2k4VAIwIQoP5DoO06waLBffvLIAdPPKYsx71K67OoGG2svc7duC/+5qf1x
 =hCS7
 -----END PGP ARMORED FILE-----
 EOF
-curl -OLs https://download.qemu.org/qemu-5.1.0.tar.xz
-gpg --verify qemu-5.1.0.tar.xz.sig
+    curl -OLs https://download.qemu.org/qemu-5.1.0.tar.xz
+    gpg --verify qemu-5.1.0.tar.xz.sig
+
+    tar -xf qemu-5.1.0.tar.xz
+fi
 
-tar -xf qemu-5.1.0.tar.xz
 cd qemu-5.1.0
-./configure --target-list=aarch64-softmmu,arm-softmmu,i386-softmmu,riscv32-softmmu,riscv64-softmmu,x86_64-softmmu
-make -j${NUM_CORE}
+./configure --target-list=${target_list}

Review comment:
       no, because the target list could be updated.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a change in pull request #8156: [QEMU] Add number of cores, target list for build

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#discussion_r641828051



##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -16,22 +16,46 @@
 # specific language governing permissions and limitations
 # under the License.
 
+#
+# Install QEMU on Ubuntu.
+#
+# Usage: ubuntu_install_qemu.sh [--target-list target0,target1,...]
+#   --target-list is list of target for QEMU comma seperated. e.g. aarch64-softmmu,arm-softmmu,...
+#
+
+set -e
+set -o pipefail
+
+QEMU_DIR=qemu-5.1.0
+
+# Get number of cores for build
 if [[ "${CI_NUM_CORE}" ]]; then

Review comment:
       done.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on a change in pull request #8156: [QEMU] Add number of cores, target list for build

Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#discussion_r645834422



##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -16,10 +16,32 @@
 # specific language governing permissions and limitations
 # under the License.
 
+#
+# Install QEMU on Ubuntu.
+#
+# Usage: ubuntu_install_qemu.sh [--target-list target0,target1,...]
+#   --target-list is list of target for QEMU comma seperated. e.g. aarch64-softmmu,arm-softmmu,...
+#
+
 set -e
-set -u
 set -o pipefail
 
+# Get number of cores for build
+if [[ "${TVM_CI_NUM_CORES}" ]]; then

Review comment:
       i think you need e.g. `if [ -n "${TVM_CI_NUM_CORES}" ]` or some operator, no?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on pull request #8156: [Docker] Add environment variable for number of cores

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#issuecomment-849997806


   cc @areusch @leandron 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch merged pull request #8156: [QEMU] Add number of cores, target list for build

Posted by GitBox <gi...@apache.org>.
areusch merged pull request #8156:
URL: https://github.com/apache/tvm/pull/8156


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on a change in pull request #8156: [QEMU] Add number of cores, target list for build

Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#discussion_r643263564



##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -16,22 +16,46 @@
 # specific language governing permissions and limitations
 # under the License.
 
+#
+# Install QEMU on Ubuntu.
+#
+# Usage: ubuntu_install_qemu.sh [--target-list target0,target1,...]
+#   --target-list is list of target for QEMU comma seperated. e.g. aarch64-softmmu,arm-softmmu,...
+#
+
+set -e
+set -o pipefail
+
+QEMU_DIR=qemu-5.1.0
+
+# Get number of cores for build
 if [[ "${CI_NUM_CORE}" ]]; then
-  NUM_CORE=${CI_NUM_CORE}
+  num_cores=${CI_NUM_CORE}
 else
-  NUM_CORE=2
+  num_cores=2
 fi
 
-set -e
-set -u
-set -o pipefail
+# Set target list for QEMU
+if [ "$1" == "--target-list" ]; then
+    shift
+    target_list=$1
+else
+    target_list="aarch64-softmmu,arm-softmmu,i386-softmmu,riscv32-softmmu,riscv64-softmmu,x86_64-softmmu"
+fi
 
-sudo sed -i '/deb-src/s/^# //' /etc/apt/sources.list
-apt update
-apt-get -y build-dep qemu
+# Check if QEMU already built
+rebuild=0

Review comment:
       ah i see. okay, do you intend to push that update soon? otherwise, may be better to keep this file simple. i agree it's a nice feature, but wondering how much extra it adds to the base image.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a change in pull request #8156: [QEMU] Add number of cores, target list for build

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#discussion_r641829859



##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -16,22 +16,46 @@
 # specific language governing permissions and limitations
 # under the License.
 
+#
+# Install QEMU on Ubuntu.
+#
+# Usage: ubuntu_install_qemu.sh [--target-list target0,target1,...]
+#   --target-list is list of target for QEMU comma seperated. e.g. aarch64-softmmu,arm-softmmu,...
+#
+
+set -e
+set -o pipefail
+
+QEMU_DIR=qemu-5.1.0
+
+# Get number of cores for build
 if [[ "${CI_NUM_CORE}" ]]; then
-  NUM_CORE=${CI_NUM_CORE}
+  num_cores=${CI_NUM_CORE}
 else
-  NUM_CORE=2
+  num_cores=2
 fi
 
-set -e
-set -u
-set -o pipefail
+# Set target list for QEMU
+if [ "$1" == "--target-list" ]; then
+    shift
+    target_list=$1
+else
+    target_list="aarch64-softmmu,arm-softmmu,i386-softmmu,riscv32-softmmu,riscv64-softmmu,x86_64-softmmu"
+fi
 
-sudo sed -i '/deb-src/s/^# //' /etc/apt/sources.list
-apt update
-apt-get -y build-dep qemu
+# Check if QEMU already built
+rebuild=0

Review comment:
       The reason is that we could update the RVM on vagrant cloud with a version that has qemu already installed with limited targets. And then if someone is using this locally, it will only rebuild with new set of targets.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on a change in pull request #8156: [QEMU] Add number of cores, target list for build

Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#discussion_r641821198



##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -16,22 +16,46 @@
 # specific language governing permissions and limitations
 # under the License.
 
+#
+# Install QEMU on Ubuntu.
+#
+# Usage: ubuntu_install_qemu.sh [--target-list target0,target1,...]
+#   --target-list is list of target for QEMU comma seperated. e.g. aarch64-softmmu,arm-softmmu,...
+#
+
+set -e
+set -o pipefail
+
+QEMU_DIR=qemu-5.1.0
+
+# Get number of cores for build
 if [[ "${CI_NUM_CORE}" ]]; then

Review comment:
       let's pick either CORE or CORES and stick with it. Prefer NUM_CORES since it's more natural

##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -16,22 +16,46 @@
 # specific language governing permissions and limitations
 # under the License.
 
+#
+# Install QEMU on Ubuntu.
+#
+# Usage: ubuntu_install_qemu.sh [--target-list target0,target1,...]
+#   --target-list is list of target for QEMU comma seperated. e.g. aarch64-softmmu,arm-softmmu,...
+#
+
+set -e
+set -o pipefail
+
+QEMU_DIR=qemu-5.1.0
+
+# Get number of cores for build
 if [[ "${CI_NUM_CORE}" ]]; then
-  NUM_CORE=${CI_NUM_CORE}
+  num_cores=${CI_NUM_CORE}
 else
-  NUM_CORE=2
+  num_cores=2
 fi
 
-set -e
-set -u
-set -o pipefail
+# Set target list for QEMU
+if [ "$1" == "--target-list" ]; then
+    shift
+    target_list=$1
+else
+    target_list="aarch64-softmmu,arm-softmmu,i386-softmmu,riscv32-softmmu,riscv64-softmmu,x86_64-softmmu"

Review comment:
       comment where this list comes from

##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -16,22 +16,46 @@
 # specific language governing permissions and limitations
 # under the License.
 
+#
+# Install QEMU on Ubuntu.
+#
+# Usage: ubuntu_install_qemu.sh [--target-list target0,target1,...]
+#   --target-list is list of target for QEMU comma seperated. e.g. aarch64-softmmu,arm-softmmu,...
+#
+
+set -e
+set -o pipefail
+
+QEMU_DIR=qemu-5.1.0
+
+# Get number of cores for build
 if [[ "${CI_NUM_CORE}" ]]; then
-  NUM_CORE=${CI_NUM_CORE}
+  num_cores=${CI_NUM_CORE}
 else
-  NUM_CORE=2
+  num_cores=2
 fi
 
-set -e
-set -u
-set -o pipefail
+# Set target list for QEMU
+if [ "$1" == "--target-list" ]; then
+    shift
+    target_list=$1
+else
+    target_list="aarch64-softmmu,arm-softmmu,i386-softmmu,riscv32-softmmu,riscv64-softmmu,x86_64-softmmu"
+fi
 
-sudo sed -i '/deb-src/s/^# //' /etc/apt/sources.list
-apt update
-apt-get -y build-dep qemu
+# Check if QEMU already built
+rebuild=0

Review comment:
       why do we need this logic in this file?

##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -45,14 +69,16 @@ p5ez/+2k4VAIwIQoP5DoO06waLBffvLIAdPPKYsx71K67OoGG2svc7duC/+5qf1x
 =hCS7
 -----END PGP ARMORED FILE-----
 EOF
-curl -OLs https://download.qemu.org/qemu-5.1.0.tar.xz
-gpg --verify qemu-5.1.0.tar.xz.sig
+    curl -OLs https://download.qemu.org/qemu-5.1.0.tar.xz
+    gpg --verify qemu-5.1.0.tar.xz.sig
+
+    tar -xf qemu-5.1.0.tar.xz
+fi
 
-tar -xf qemu-5.1.0.tar.xz
 cd qemu-5.1.0
-./configure --target-list=aarch64-softmmu,arm-softmmu,i386-softmmu,riscv32-softmmu,riscv64-softmmu,x86_64-softmmu
-make -j${NUM_CORE}
+./configure --target-list=${target_list}
+make -j${num_cores}
 sudo make install
 
-# For debugging with qemu
+# # For debugging with qemu

Review comment:
       nit: revert this change

##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -16,22 +16,46 @@
 # specific language governing permissions and limitations
 # under the License.
 
+#
+# Install QEMU on Ubuntu.
+#
+# Usage: ubuntu_install_qemu.sh [--target-list target0,target1,...]
+#   --target-list is list of target for QEMU comma seperated. e.g. aarch64-softmmu,arm-softmmu,...
+#
+
+set -e
+set -o pipefail
+
+QEMU_DIR=qemu-5.1.0
+
+# Get number of cores for build
 if [[ "${CI_NUM_CORE}" ]]; then
-  NUM_CORE=${CI_NUM_CORE}
+  num_cores=${CI_NUM_CORE}
 else
-  NUM_CORE=2
+  num_cores=2
 fi
 
-set -e
-set -u
-set -o pipefail
+# Set target list for QEMU
+if [ "$1" == "--target-list" ]; then
+    shift
+    target_list=$1
+else
+    target_list="aarch64-softmmu,arm-softmmu,i386-softmmu,riscv32-softmmu,riscv64-softmmu,x86_64-softmmu"
+fi
 
-sudo sed -i '/deb-src/s/^# //' /etc/apt/sources.list
-apt update
-apt-get -y build-dep qemu
+# Check if QEMU already built
+rebuild=0
+if [ -d ${QEMU_DIR} ]; then
+    rebuild=1
+fi
+
+if [ $rebuild -eq 0 ]; then
+    sudo sed -i '/deb-src/s/^# //' /etc/apt/sources.list

Review comment:
       i think you should at least filter by the particular deb-src line you want to re-enable? also comment why this is needed

##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -45,14 +69,16 @@ p5ez/+2k4VAIwIQoP5DoO06waLBffvLIAdPPKYsx71K67OoGG2svc7duC/+5qf1x
 =hCS7
 -----END PGP ARMORED FILE-----
 EOF
-curl -OLs https://download.qemu.org/qemu-5.1.0.tar.xz
-gpg --verify qemu-5.1.0.tar.xz.sig
+    curl -OLs https://download.qemu.org/qemu-5.1.0.tar.xz
+    gpg --verify qemu-5.1.0.tar.xz.sig
+
+    tar -xf qemu-5.1.0.tar.xz
+fi
 
-tar -xf qemu-5.1.0.tar.xz
 cd qemu-5.1.0
-./configure --target-list=aarch64-softmmu,arm-softmmu,i386-softmmu,riscv32-softmmu,riscv64-softmmu,x86_64-softmmu
-make -j${NUM_CORE}
+./configure --target-list=${target_list}

Review comment:
       seems like if rebuild should also gate these? should it be renamed?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mehrdadh commented on a change in pull request #8156: [QEMU] Add number of cores, target list for build

Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#discussion_r645842591



##########
File path: docker/install/ubuntu_install_qemu.sh
##########
@@ -16,10 +16,32 @@
 # specific language governing permissions and limitations
 # under the License.
 
+#
+# Install QEMU on Ubuntu.
+#
+# Usage: ubuntu_install_qemu.sh [--target-list target0,target1,...]
+#   --target-list is list of target for QEMU comma seperated. e.g. aarch64-softmmu,arm-softmmu,...
+#
+
 set -e
-set -u
 set -o pipefail
 
+# Get number of cores for build
+if [[ "${TVM_CI_NUM_CORES}" ]]; then

Review comment:
       shoot, it was working without `-n`, that's why I missed it. But this is a better syntax.
   Thanks for finding this!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] areusch commented on pull request #8156: [QEMU] Add number of cores, target list for build

Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #8156:
URL: https://github.com/apache/tvm/pull/8156#issuecomment-856074433


   thanks @mehrdadh , the PR is now merged!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org