Closed
Bug 1122598
Opened 10 years ago
Closed 10 years ago
Reorganize image definitions in testing/docker to support non-b2g builds
Categories
(Taskcluster :: General, defect)
Taskcluster
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: dustin, Assigned: dustin)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
dustin
:
review+
garndt
:
feedback+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
As a note to self, I want to improve the README in testing/docker while I'm at it.
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8550368 -
Flags: review?(jopsen)
Comment 3•10 years ago
|
||
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+
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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 6•10 years ago
|
||
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+
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
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 :)
Assignee | ||
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
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)
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
Comment on attachment 8551788 [details] [diff] [review]
bug1122698.patch
Clearing f?, commented directly in the bug.
Attachment #8551788 -
Flags: feedback?(garndt) → feedback+
Assignee | ||
Comment 12•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 13•10 years ago
|
||
Updated•9 years ago
|
Component: TaskCluster → General
Product: Testing → Taskcluster
Target Milestone: --- → mozilla41
Version: unspecified → Trunk
Comment 14•9 years ago
|
||
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.
Description
•