-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[Don't MERGE] CI 2.0, revamp ci scripts and dockerfiles, integrate IoT builds #9943
Conversation
|
||
def get_platforms(path: Optional[str]="docker"): | ||
"""Get a list of architectures given our dockerfiles""" | ||
dockerfiles = glob.glob(os.path.join(path, "Dockerfile.build.*")) |
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.
You can exclude some files with patterns directly:
glob.glob(os.path.join(path, "Dockerfile.build.*[!~]"))
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.
Reuse get_dockerfile with this pattern?
if use_nvidia_docker: | ||
return "nvidia-docker" | ||
else: | ||
return "docker" |
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 we use nvidia-docker always?
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.
No, it's not available on instances without GPUs
# We need to create it first, otherwise it will be created by the docker daemon with root only permissions | ||
os.makedirs(local_build_folder, exist_ok=True) | ||
logging.info("Running %s in container %s", command, tag) | ||
runlist = [docker_binary, 'run', '--rm', |
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.
Shouldn't we use --init here?
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.
Added, thanks
|
||
set -ex | ||
|
||
clean_repo() { |
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.
Nice! I wonder if that would still work if files would be created under root?
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.
It shouldn't since the container is running as restricted user
then | ||
$@ | ||
else | ||
cat<<EOF |
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.
is EOF correct?
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, it's just a label describing where a multiline string starts and ends. You could also call it ANTON or whatever you feel like. It just has to be matched by the same label (in this case line 251)
I created a new PR at #9946 as I don't have write access to pedros repo and I'll take care of the migration and integration |
Description
Checklist
Essentials
make lint
)Changes
Comments