On Fri, 2019-03-15 at 15:06 +0000, Daniel P. Berrangé wrote:
On Fri, Mar 15, 2019 at 01:24:20PM +0100, Andrea Bolognani wrote:
[...]
+# Run in a separate build directory. Set to empty +# for a in-source tree build, but note SRCDIR must +# also be set to a corresponding relative path +VPATHDIR = vpath +SRCDIR = ..
Do we even care about in-source builds? It's usually VPATH builds that will end up broken by mistake because someone was not careful enough when tweaking the build system...
As long as in-source builds are permitted by libvirt, we must provide support for using them here to reproduce problems.
Personally I think we should drop support for in-source builds as having 2 different ways of building the same thing means we frequently break one or the other. There were objections last time I suggested that though. NB QEMU is about to drop support for in-source builds to reduce breakage
I'm aware of the conversation happening in QEMU land, and in fact that's at least in part what caused the question :) But you're right, as long as we support both in libvirt we should also provide a way to pick one or the other when (CI) building.
Either way, since we have full control over the way local directories will show up inside the container, I would define these as
SRCDIR = /build VPATHDIR = $(SRCDIR)/vpath
that way people will only ever have to override VPATHDIR.
I'm not sure that will work in all places I use these vars, but will investigate.
I have mentioned some potential issues later in the mail, but along with those I have also provided solutions so I think we should be good :)
+# Docker doesn't require the IDs you run as to exist in +# the container's /etc/passwd & /etc/group files, but +# if they do not, then libvirt's 'make check' will fail +# many tests +PWDB_MOUNTS = \ + --volume $(HERE)/citree/group:/etc/group:ro,z \ + --volume $(HERE)/citree/passwd:/etc/passwd:ro,z
Why do we copy the files under /citree before handing them over to Docker? Shouldn't we be able to mount them directly off /etc?
Prepare for a story of pain and suffering and swearing....
I did indeed just pass /etc/passwd direct to the --volume arg when i first wrote this and it all worked "fine".
Then my screen locked and I couldn't log back in either through GDM or the Linux text console or SSH
I had to reboot, at which point almost every single systemd service failed.
Single user mode wouldn't even work.
After booting single user mode with SELinux disabled I discovered that /etc/passwd now had the SELinux label of the container I had just run, instead of "passwd_file_t"
Thus no process confined by SELinux had permission to read /etc/passwd making life rather difficult.
There's probably a way to get docker to not screw with the SELinux labels but I think it is more prudent to take the absolutely guaranteed safe option & copy the file :-)
Ouch :( Perhaps we could add a very short comment mentioning the reason for making a copy of the files, for the benefit of whoever will touch the script months down the line. [...]
+GIT_ARGS = \ + -c advice.detachedHead=false \ + -q \
Why quiet? We're pretty verbose throughout the rest of the process.
I don't think the message from git were useful - the other stuff you see is all related to the actual build process which is the interesting bit.
Alright. [...]
As for CONFIGURE_OPTS, I think the name is pretty poor because it's too similar to CONFIGURE_ARGS... We could either rename it to something like CONFIGURE_IMAGE_ARGS, since it contains the arguments that are baked in the Docker image, or CONFIGURE_CROSS_ARGS, since we have no planned use (that I know of) outside of passing information specific to cross-compilation. I think I have a slight preference for the former, but either one would be an improvement.
I don't think the name similarity actually matters in practice because only one of them is used by the developer, the other is just internal to the image, and we've documented which to use higher up in the makefile. So I'd rather just stick with the more concise names.
I'd really prefer a name that removes the ambiguity. [...]
One more question: what is gl_public_submodule_commit= supposed to do? I tried running builds both with and without it, and the results seem to be the same.
gnulib has a make check target that will fail as the way we have cloned submodules isn't the normal way submodules are supposed to be initialized by git.
Hm, can you double check please? As mentioned above, I tried both with and without the make variable being set, and in both cases gnulib's 'make check' passed. [...]
+ @echo "Available targets:" + @echo + @echo " cibuild-\$$IMAGE - run a default 'make'" + @echo " cicheck-\$$IMAGE - run a 'make check'" + @echo " cishell-\$$IMAGE - run an interactive shell"
Just a thought: instead of
make ci-build-centos-7 MAKE_ARGS=check
and in the future
make ci-build-debian-9-cross-aarch64
would it make sense to have something like
make ci-build OS=centos-7 MAKE_ARGS=check make ci-build OS=debian-9 CROSS=aarch64
instead? A bit more typing, perhaps, but it looks kinda better in my opinion, with the variable parts clearly presented as such...
I rather prefer the more concise target names - I don't think it really adds anything to use variables
I disagree on concise: they're definitely shorter, but that's because all the information is squished together, which makes it harder to parse at a glance. When naming Docker images we don't have much of a choice, because we have pretty much the same restrictions as when naming files, but that's not the case here so we could do better... With that said, if you feel strongly about the current names I won't stand in the way :) -- Andrea Bolognani / Red Hat / Virtualization