Update Dockerfile and add UID/GID support #231

gvnn3 · 2021-08-12T16:30:49Z

Update the Dockerfile to reference more recent versions of Ubuntu and tools.

Add support for docker-uid/docker-gid config values to be used when running the docker image.

…cker.

jrtc27 · 2021-08-12T16:34:39Z

docker/Dockerfile

+# INSTALL clang 10.0
 RUN wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | apt-key add -
 RUN echo "deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic-8 main" > /etc/apt/sources.list.d/llvm.list
 # RUN echo "deb http://ppa.launchpad.net/ubuntu-toolchain-r/test/ubuntu trusty main" > /etc/apt/sources.list.d/r-toolchain.list
 # RUN apt-key adv --keyserver keyserver.ubuntu.com --recv-keys 1E9377A2BA9EF27F
-RUN apt-get update && apt-get install -y clang-8 lld-8
+RUN apt-get update && apt-get install -y clang-10 lld-10


Ubuntu 20.04 itself has LLVM 10 as the default version, you shouldn't need to add the external repository (especially not one built for Ubuntu 18.04), nor should you need to install versioned packages

jrtc27 · 2021-08-12T16:38:45Z

docker/Dockerfile


 RUN apt-get update && apt-get install -y cmake

+RUN groupadd -g 12345 -r cheri && useradd -u 12345 --no-log-init -r -g cheri cheri


No home directory? Also, adduser is preferred and can create both the group and user in one command

jrtc27 · 2021-08-12T16:39:14Z

pycheribuild/__main__.py

+                        docker_run_cmd = ["docker", "run", "--user", cheri_config.docker_uid + ":" + cheri_config.docker_gid,
+                                          "--rm", "--interactive", "--tty"] + docker_dir_mappings
+                else:
+                        docker_run_cmd = ["docker", "run", "--user", str(os.getuid()) + ":" + str(os.getgid()),


Use variables to avoid the duplication

jrtc27 · 2021-08-12T16:39:30Z

pycheribuild/__main__.py

@@ -198,8 +198,12 @@ def real_main():
                subprocess.check_call(start_cmd)
                docker_run_cmd = ["docker", "exec", cheri_config.docker_container] + cheribuild_args
            else:
-                docker_run_cmd = ["docker", "run", "--user", str(os.getuid()) + ":" + str(os.getgid()),
-                                  "--rm", "--interactive", "--tty"] + docker_dir_mappings
+                if cheri_config.docker_uid != "":


What if I specify a UID but not a GID?

jrtc27 · 2021-08-12T16:40:09Z

pycheribuild/config/chericonfig.py

@@ -278,6 +278,8 @@ def __init__(self, loader, action_class):
                                                             help="Attach to the same container again (note: "
                                                                  "docker-container option must be an id rather than "
                                                                  "a container name")
+        self.docker_uid = loader.add_option("docker-uid", help="UID to under Docker", default="", group=loader.docker_group)


Missing a word? Also why "" rather than None? And could you not just default this to os.get[ug]id() so you don't need to check whether it's set?

jrtc27 · 2021-08-12T16:42:12Z

pycheribuild/projects/release.py

+class RISCV64Release(Release):
+    target = "riscv64-release"
+    dependencies = ["gdb-native", "qemu",
+                    "bbl-baremetal-riscv64-purecap",


BBL should be implied by QEMU

It's only a dependency of run-riscv64{,-purecap,-hybrid} so is needed here.

Huh. I can kinda see why (especially if you don't want to use qemu-system-riscv64cheri but a different one instead), but it's a bit surprising given QEMU's source knows about it.

jrtc27 · 2021-08-12T16:43:24Z

pycheribuild/projects/release.py

+            """)
+
+
+class MorelloRelease(Release):


Would these not be better as a single "CrossCompile" project so you have a single class that does multiple architectures?

jrtc27 · 2021-08-12T16:45:01Z

pycheribuild/projects/release.py

+        cls.gittag = cls.add_config_option("gittag", help="Tag to apply")
+
+    def __init__(self, config: CheriConfig):
+        super().__init__(config)


These aren't needed if you're not doing anything in the overridden constructor

jrtc27 · 2021-08-12T16:45:36Z

pycheribuild/projects/release.py

+    def process(self):
+        source_root = self.config.source_root
+
+        Path(source_root, "cheribuild/cheribuild.json").write_text("""{


These will have side-effects in --pretend mode I believe, and thus during the "pretend build everything" test

jrtc27 · 2021-08-12T16:50:02Z

pycheribuild/projects/release.py

+        source_root = self.config.source_root
+
+        Path(source_root, "cheribuild/cheribuild.json").write_text("""{
+            "source-root": "../../sources",


Is this evaluated relative to cheribuild.py or the current working directory?

Also, this results in a non-standard layout; the standard layout is that build-root is source-root/build and output-root is source-root/output.

(answer to the first part: if loaded from a config file, relative to the file, otherwise if on the command line it's relative to the current working directory)

In hindsight I should have used a separate sources directory, but unfortunately it's too late to change that. I'm okay with doing it for the docker-based release since people using it are starting with a clean environment

So, the reason I'm of the view we shouldn't is not a technical one but for our (and their) sanity. All our documentation talks about ~/cheri/cheribuild, ~/cheri/$project, ~/cheri/build and ~/cheri/output. If we suddenly have two different default layouts that people are exposed to they're going to mix things up and get confused, and we'll have to have documentation for them both everywhere. Using the same layout allows all our documentation to always be simple and consistent, with the only thing we need to note being that ~/cheri may (should) be somewhere else for releases to avoid conflicts (ie ~/cheri-rel-YYYYMMDD or some such, or possibly ~/cheri-rel-YYYYMMDD/cheri given the current tarball, which is a bit ugly but in a way perhaps somewhat sensible as then it looks even more like normal).

jrtc27 · 2021-08-12T16:54:57Z

pycheribuild/projects/release.py

+            "output/morello-sdk/firmware",
+            "output/cheribsd-morello-purecap.img",
+            "output/sdk",
+            "sources/cheribuild",


Only because you've configured your cheribuild to have a non-standard layout. If I were to run this it wouldn't work. If you want a specific layout you need to assemble that yourself in a staging directory, but IMO the layout you pick should be the standard one.

jrtc27 · 2021-08-12T16:58:43Z

pycheribuild/projects/release.py

+            "--exclude=*.git",
+            "output/morello-sdk/firmware",
+            "output/cheribsd-morello-purecap.img",
+            "output/sdk",


Missing the rootfs

jrtc27 · 2021-08-12T17:00:40Z

pycheribuild/projects/release.py

+        exec "${dir}/cheribuild.py" install-morello-fvp run-fvp-morello-purecap "$@""")
+        install_script.chmod(0o755)
+
+        run_command("bsdtar", "-cavf", output_root / "release-morello.tar.xz", 


Should we be putting -purecap in the names? For Morello it's perhaps unnecessary as foo-morello doesn't exist, only foo-morello-{hybrid,purecap}, so one can infer it's purecap, but for RISC-V we do have foo-riscv64 without any suffix that it might get confused with, and we should probably be consistent.

jrtc27 · 2021-08-12T17:03:21Z

pycheribuild/projects/release.py

+
+
+class MorelloRelease(Release):
+    target = "morello-release"


Normally we only put the architecture as a prefix when you're building a native tool, like morello-qemu (until merged into the main CHERI QEMU) or morello-llvm; when you're producing artifacts to run on that architecture it's instead a suffix.

gvnn3 and others added 9 commits June 2, 2021 11:19

First draft of tagging and release commands.

2926b21

Make the repos list global.

2c310d7

Clean up paths, get the right names for things.

5543378

Abrtract away more details to the parents classes.

e12fdaf

First working version of the RISCV-64 release target.

7701f23

Merge remote-tracking branch 'origin/master' into gnn-release

5f3ee93

Address updates to project_name and target

f60e635

Merge remote-tracking branch 'origin/master' into gnn-release

c6ca06d

Update to latest Ubuntu and tools. Add UID/GID support for running Do…

3133912

…cker.

jrtc27 reviewed Aug 12, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Dockerfile and add UID/GID support #231

Update Dockerfile and add UID/GID support #231

gvnn3 commented Aug 12, 2021

jrtc27 Aug 12, 2021

jrtc27 Aug 12, 2021

jrtc27 Aug 12, 2021

jrtc27 Aug 12, 2021

jrtc27 Aug 12, 2021

jrtc27 Aug 12, 2021

arichardson Aug 26, 2021 •

edited

Loading

jrtc27 Aug 26, 2021

jrtc27 Aug 12, 2021

jrtc27 Aug 12, 2021

jrtc27 Aug 12, 2021

jrtc27 Aug 12, 2021 •

edited

Loading

jrtc27 Aug 13, 2021

arichardson Aug 26, 2021

jrtc27 Aug 26, 2021 •

edited

Loading

jrtc27 Aug 12, 2021 •

edited

Loading

jrtc27 Aug 12, 2021

jrtc27 Aug 12, 2021

jrtc27 Aug 12, 2021


		RUN apt-get update && apt-get install -y cmake

		RUN groupadd -g 12345 -r cheri && useradd -u 12345 --no-log-init -r -g cheri cheri

Update Dockerfile and add UID/GID support #231

Are you sure you want to change the base?

Update Dockerfile and add UID/GID support #231

Conversation

gvnn3 commented Aug 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichardson Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

jrtc27 Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichardson Aug 26, 2021 •

edited

Loading

jrtc27 Aug 12, 2021 •

edited

Loading

jrtc27 Aug 26, 2021 •

edited

Loading

jrtc27 Aug 12, 2021 •

edited

Loading