Closed
Bug 974082
Opened 11 years ago
Closed 11 years ago
Don't reset mock environments if we don't have to
Categories
(Release Engineering :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: catlee, Assigned: catlee)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
text/x-patch
|
bhearsum
:
review+
catlee
:
checked-in+
|
Details |
(deleted),
patch
|
RyanVM
:
review+
catlee
:
checked-in+
|
Details | Diff | Splinter Review |
Most of our Linux based builds use mock to manage a chroot environment to do the build in. Currently we always reset and reinitialize the mock environment at the beginning of each build, even if we're going to install exactly the same packages over.
We should stop that! mock environments should only be reset when required.
Related: we should make the packages for different build types the same as much as possible, to avoid resetting when we don't need to.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → catlee
Assignee | ||
Comment 1•11 years ago
|
||
The idea here is to put a file inside the mock environment that contains a hash of the list of packages installed. When we start a build, look at the file, and if it's the same as our list of packages, we can skip re-initializing the mock environment.
We still install packages in case there are new packages in the repos, but it's pretty quick (takes about 2s).
Attachment #8378267 -
Flags: review?(bhearsum)
Comment 2•11 years ago
|
||
Comment on attachment 8378267 [details] [diff] [review]
fastmock-.patch
Review of attachment 8378267 [details] [diff] [review]:
-----------------------------------------------------------------
::: mozharness/mozilla/mock.py
@@ +35,5 @@
>
> + def delete_mock_files(self, mock_target, files):
> + """Delete files from the mock environment `mock_target`. `files` should
> + be an iterable of 2-tuples: (src, dst). Only the dst component is
> + deleted."""
What's the point in files being a tuples? Seems like this should just receive a simple iterable.
@@ +137,5 @@
> + mock_root = self.get_output_from_command(
> + ['mock_mozilla', '-r', mock_target, '--print-root-path'])
> + package_hash_file = os.path.join(mock_root, "builds/package_list.hash")
> + if os.path.exists(package_hash_file):
> + old_packages = self.read_from_file(package_hash_file)
s/old_packages/old_packages_hash/g?
@@ +146,3 @@
>
> + package_list_hash = hashlib.new('sha1')
> + for p in sorted(mock_packages):
This code falls back on c['mock_packages'] if mock_packages is None. It seems like you should be doing that here too - otherwise this won't work for scripts like servo_build.py, where mock_packages are defined in the config.
@@ +149,5 @@
> + package_list_hash.update(p)
> + package_list_hash = package_list_hash.hexdigest()
> +
> + did_init = True
> + if old_packages != package_list_hash:
We talked a lot in person about what to do about packages with dependencies. It looks like you've decided not to take them into account, and only deal with explicitly listed packages. Are you worried at all about things breaking if a dependency changes?
Assignee | ||
Comment 3•11 years ago
|
||
I think this addresses your concerns. The function signature for delete_files is to make it easy to pass in the mock_files structure, which is already a list of tuples.
Attachment #8378267 -
Attachment is obsolete: true
Attachment #8378267 -
Flags: review?(bhearsum)
Attachment #8378336 -
Flags: review?(bhearsum)
Comment 5•11 years ago
|
||
Copy/Pasting an irc convo here, (note pastebins won't last very long, but theorycraft discussed is still useful)
[11:32:45] Callek catlee: for mock_ init stuff, depending on if deplist is a good thing vs time investment you might consider using `repoquery --tree-requires`
[11:33:02] Callek catlee: which will give you full deps and versions of said deps
[11:33:34] =-= coop|mtg is now known as coop
[11:33:40] Callek example output: https://callek.pastebin.mozilla.org/4338513
** ED NOTE: $ repoquery --tree-requires gcc
[11:35:37] Callek or you could do it the simple way if you want to concat a list for the hash: https://callek.pastebin.mozilla.org/4338522
** ED NOTE: $ repoquery -R --recursive gcc
[11:38:28] catlee Callek: huh, cool
[11:38:37] catlee so that would need to run inside mock I think?
[11:38:46] catlee or at least with the mock env's rpmdb
[11:38:52] Callek catlee: yea I think so
[11:39:16] Callek I haven't tested the theorycraft here, but it actually queries against the yum repo, not local installs
[11:39:59] catlee yeah
[11:40:07] catlee I wonder if that's worthwhile...
[11:40:07] Callek there is also | --installed limit query to installed pkgs only|
[11:40:22] Callek and lots of other options
[11:44:00] Callek catlee: for set-in-stone docs, I'm copy-pasting the convo above to bug, I'll leave up to you/others on the "is it worthwhile" front
[11:44:09] catlee sure
Updated•11 years ago
|
Attachment #8378336 -
Flags: review?(bhearsum) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8378336 -
Flags: checked-in+
Assignee | ||
Comment 6•11 years ago
|
||
In production
Assignee | ||
Comment 7•11 years ago
|
||
And a bustage fix:
http://hg.mozilla.org/build/mozharness/rev/eccc8fbf5f95
Assignee | ||
Comment 8•11 years ago
|
||
this is the only difference between the other 64-bit mock environments. adding libxml2 to these configs means we should avoid having to reset mock for any sequential b2g device builds.
Attachment #8382280 -
Flags: review?(ryanvm)
Comment 9•11 years ago
|
||
Comment on attachment 8382280 [details] [diff] [review]
add libxml2 to emulator/emulator-ics
Review of attachment 8382280 [details] [diff] [review]:
-----------------------------------------------------------------
Looks like only mako/config.json and emulator-jb/config.json already include libxml2. Should the other devices as well?
Attachment #8382280 -
Flags: review?(ryanvm) → review+
Assignee | ||
Comment 10•11 years ago
|
||
I think all the other devices are using the 32-bit environment. The package lists don't need to be the same between the environments for this optimization to work.
Assignee | ||
Updated•11 years ago
|
Attachment #8382280 -
Flags: checked-in+
Comment 11•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•