Closed Bug 1122598 Opened 5 years ago Closed 5 years ago

Reorganize image definitions in testing/docker to support non-b2g builds

Categories

(Taskcluster :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dustin, Assigned: dustin)

References

Details

Attachments

(1 file, 1 obsolete file)

To support building Fennec in taskcluster, we need some images that are more generic.  base-builder is a little too b2g-specific.  So, I'd like to split the images up a little bit.
As a note to self, I want to improve the README in testing/docker while I'm at it.
Attached patch split-base-build.patch (obsolete) — Splinter Review
Attachment #8550368 - Flags: review?(jopsen)
Comment on attachment 8550368 [details] [diff] [review]
split-base-build.patch

Review of attachment 8550368 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fairly reasonable to me.
I don't know how much `groupinstall -y "Development Tools"`, but it might be worth checking that
things like screen, curl, wget, tar, zip, gzip and possibly vim are installed.
It's really annoying to debug images without these tools :)
(And we'll likely need most, if not all of them later.   

Not sure I should review this, as I haven't hacked these since they landed in alder, but r+ anyways :)

::: testing/docker/base-build/system-setup.sh
@@ -138,5 @@
> -
> -# Install gcc to build gecko
> -rpm -ih $base_url/gcc473_0moz1-4.7.3-0moz1.x86_64.rpm
> -
> -### Clean up from setup

Any reason to remove the clean up step.
Ie. deleting of cached packages and deleting of setup.sh script?

Note, not cleaning the package cache in this script and doing it later (say in another step or docker image) means that the layer resulting from this step/script will contain the cached packages, even if they are deleted later.

::: testing/docker/build.sh
@@ +62,5 @@
>      $folder/build.sh -t $tag $*
>    else
> +    # use --no-cache so that we always get the latest updates from yum
> +    # and use the latest version of system-setup.sh
> +    docker build --no-cache -t $tag $folder

Nice! :)
Attachment #8550368 - Flags: review?(jopsen) → review+
Slightly, related:
I've heard jlal rant about not liking system-setup.sh. I'm not the biggest fan either.
But there is a valid argument for it, namely that everything done in a single docker command/step
becomes a single layer. So if you have 3 steps:
 1. download package
 2. install package
 3. delete cached package
The package will not be in the system, but it'll still take up space in the layers, after these 3 steps.
And, hence, have to be downloaded when you use the resulting image. Unless we someday starts squashing layers.
If you combine the 3 steps into one step, the layer won't contain the cached package.
One can also write multi-line steps in the Dockerfile using "\" (but it's not very fun). 
So I'm not sure we should discourage use of the system-setup.sh pattern, before we have a better alternative.

@jlal, thoughts? I know you want to kill system-setup.sh :)
But do we have the right alternative?
Flags: needinfo?(jlal)
Comment on attachment 8550368 [details] [diff] [review]
split-base-build.patch

r? wcosta since he more or less wrote/owns this version
Flags: needinfo?(jlal)
Attachment #8550368 - Flags: review?(wcosta)
Attachment #8550368 - Flags: review+
Attachment #8550368 - Flags: feedback?(garndt)
Comment on attachment 8550368 [details] [diff] [review]
split-base-build.patch

Review of attachment 8550368 [details] [diff] [review]:
-----------------------------------------------------------------

One minor nit, r+

::: testing/docker/b2g-build/Dockerfile
@@ +1,2 @@
> +FROM          quay.io/mozilla/base-build:0.0.1
> +MAINTAINER    Jonas Finnemann Jensen <jopsen@gmail.com>

I think you can assign yourself as the maintainer. We have been copying Dockerfiles from one image to another and never modify that.

::: testing/docker/builder/Dockerfile
@@ -2,1 @@
>  MAINTAINER    Jonas Finnemann Jensen <jopsen@gmail.com>

Same here, you can assign yourself as the maintainer.
Attachment #8550368 - Flags: review?(wcosta) → review+
Status: NEW → ASSIGNED
Good point regarding squashing -- I had forgotten about that.  I'll switch back to a system-setup.sh script for now (which will work fine especially now that we're running with --no-cache).  And I'll assign myself as maintainer :)
Oops, this patch already kept the system-setup.sh's in place.  My *next* patch didn't, but I'll correct that.

b2g-build.sh/system-setup.sh installs vim and screen.  I assume the remainder are available from the groupinstalls.  In any event, this patch shouldn't alter the list of leaf packages that are installed.
Attached patch bug1122698.patchSplinter Review
Carrying forward the r+ and f?garndt.  The interdiff alters maintainers, adds cleanup to base-build, moves dbus-uuidgen before the cleanup in b2g-build, and expands comments on running system-setup.sh.
Attachment #8550368 - Attachment is obsolete: true
Attachment #8550368 - Flags: feedback?(garndt)
Attachment #8551788 - Flags: review+
Attachment #8551788 - Flags: feedback?(garndt)
I like the direction that this is going and definitely agree with moving bits into more specific docker images based on build type.   

I see that utilities like tar, etc are moved into the b2g-build image.  Should those be in the base build image or is that installed by another package? If it's installed by a different package we could remove those from the b2g-build system setup.

+1 for killing system-setup.sh at some point.  Once we split out the images appropriately we could work on killing that and be smarter with what docker will cache.
Comment on attachment 8551788 [details] [diff] [review]
bug1122698.patch

Clearing f?, commented directly in the bug.
Attachment #8551788 - Flags: feedback?(garndt) → feedback+
Comment on attachment 8551788 [details] [diff] [review]
bug1122698.patch

Regarding tar, etc. -- those are in the "Base" group which is not automatically installed (it seems only "Core" is in the base centos image).  It'd certainly be reasonable to install "Base" in base-build, but let's save that for another patch.

https://hg.mozilla.org/projects/alder/rev/91164440e535
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Component: TaskCluster → General
Product: Testing → Taskcluster
Target Milestone: --- → mozilla41
Version: unspecified → Trunk
Resetting Version and Target Milestone that accidentally got changed...
Target Milestone: mozilla41 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.