-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[CI][Docker][Cortex-M]Update scripts to update ci_cortexm to Ubuntu 20.04 #13736
Conversation
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment. Generated by tvm-bot |
@tvm-bot rerun |
cc @Mousius |
70e98dd
to
c1456ae
Compare
@tvm-bot rerun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot more secure @mehrdadh 😸 one last change and then I think it's good to go.
echo deb http://apt.llvm.org/focal/ llvm-toolchain-focal-13 main\ | ||
>> /etc/apt/sources.list.d/llvm.list | ||
|
||
wget -q -O - http://apt.llvm.org/llvm-snapshot.gpg.key|sudo apt-key add - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should get a specific key with a specific fingerprint from a reasonably trust worthy key server:
wget -q -O - http://apt.llvm.org/llvm-snapshot.gpg.key|sudo apt-key add - | |
apt-key adv --keyserver keyserver.ubuntu.com --recv-keys 15CF4D18AF4F7421 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
COPY install/ubuntu1804_install_llvm.sh /install/ubuntu1804_install_llvm.sh | ||
RUN bash /install/ubuntu1804_install_llvm.sh | ||
COPY install/ubuntu2004_install_llvm.sh /install/ubuntu2004_install_llvm.sh | ||
RUN bash /install/ubuntu2004_install_llvm.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mehrdadh, thanks for this upgrade. Since Ubuntu20.04 has default installation of python3.8
and python3.7
EOL is June 2023 as per https://peps.python.org/pep-0537/#lifespan. Since I have not seen any other discussions, my assumption is that we still do wish to support older version until its EOL. We most likely would require python3.7
installation from https://launchpad.net/~deadsnakes/+archive/ubuntu/ppa.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashutosh-arm do we need python3.7 for cortexm CI image? If so, we should add it in a separate PR.
This PR doesn't update the image yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we will need to support python3.7
until its EOL. Considering this, the next thing that pops up in my brain is when the ci_cortexm
gets updated eventually corresponding to this (current) change, will the default venv still remain at python3.7 or we would move cortex-m docker image to use install/ubuntu2004_install_python.sh
?
tvm/docker/Dockerfile.ci_cortexm
Line 38 in 92da138
ENV TVM_VENV /venv/apache-tvm-py3.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
install/ubuntu2004_install_python.sh
installs python3.8 as well. We have to add another script to install 3.7 in ubuntu20.04 if we need it.
Switching the ci image to 3.8 doesn't mean we're not supporting 3.7 anymore. Could you elaborate if there is a specific reason that this image has to run 3.7? We have switched other images like hexagon to 3.8 already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, I will change the scripts to install python3.7 in ci_cortexm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
08ce7f8 addresses your concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mehrdadh
@Mousius PTAL and explicitly approve if there's no other suggestions. thanks! |
@tvm-bot rerun |
6a8124d
to
08ce7f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have had a look at the changes done for python3.7
support. LGTM! Thanks @mehrdadh for the update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the commit message @mehrdadh? The scope of this PR has expanded massively, I'd actually agree with your initial suggestion of splitting the Python 3.7 stuff into a separate PR as the changes you were trying to make are now dwarfed by unrelated ones.
@Mousius I updated both PR description which will be used for commit message and PR tilte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ok to me, thanks for your diligence on the security points here @mehrdadh 😺
…0.04 (apache#13736) - Updates nrfjprog to latest (10.18.1) to fix issues related to running microTVM tests inside docker. The main issue was that with older version commands like nrfjprog --com was not working even with docker --privileged. - Update python install script to support python 3.7 in 20.04 -> we keep using python 3.7 in ubuntu 20.04 until its EOL - Add script to install LLVM in ubuntu 20.04
This PR makes changed to update ci_cortexm to 20.04. Here is a list of changes:
cc @driazati @gromero