AU: Try delta updates first, then full updates

AU: Try delta updates first, then full updates

Also, some bug fixes.

[adlr, 2009-12-10 21:08:30]

Look at the bright side: this is the last pending review I have for you.

[Daniel Erat, 2009-12-10 23:29:10]

http://codereview.chromium.org/492008/diff/1/6
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/492008/diff/1/6#newcode344
src/platform/update_engine/delta_diff_generator.cc:344: const std::string&
old_dir,
Why this instead of adding a "using std::string" to this file like you have for
other stuff?

http://codereview.chromium.org/492008/diff/1/6#newcode349
src/platform/update_engine/delta_diff_generator.cc:349: std::vector<char>* out,
You're already pulling vector out of std:: in this file.

http://codereview.chromium.org/492008/diff/1/6#newcode369
src/platform/update_engine/delta_diff_generator.cc:369: vector<char> patch_file;
declare this down where you use it instead of way up here

http://codereview.chromium.org/492008/diff/1/6#newcode370
src/platform/update_engine/delta_diff_generator.cc:370: if (!avoid_diff) {
Since you're already returning early from a few places within this if condition,
I think it'd be cleaner to just have

if (!avoid_diff) {
  out->swap(*ret);
  return true;
}

so you don't need extra indenting.

http://codereview.chromium.org/492008/diff/1/6#newcode376
src/platform/update_engine/delta_diff_generator.cc:376: *out = *ret;
swap()

http://codereview.chromium.org/492008/diff/1/6#newcode393
src/platform/update_engine/delta_diff_generator.cc:393: const string kPatchFile
= "/tmp/delta.patch";
use mkstemp() instead

http://codereview.chromium.org/492008/diff/1/6#newcode396
src/platform/update_engine/delta_diff_generator.cc:396:
cmd.push_back("/usr/bin/bsdiff");
move this path to the top of the file

http://codereview.chromium.org/492008/diff/1/6#newcode412
src/platform/update_engine/delta_diff_generator.cc:412: *out = *ret;
swap()

http://codereview.chromium.org/492008/diff/1/6#newcode457
src/platform/update_engine/delta_diff_generator.cc:457: // here with
always_full_paths expanded.
I think that this comment would be more useful above WriteFileDiffsToDeltaFile()
in the header.

http://codereview.chromium.org/492008/diff/1/7
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/492008/diff/1/7#newcode37
src/platform/update_engine/delta_diff_generator.h:37: // as a diff. This is
useful for files that change during postinstall, since
When you say "files that change during postinstall", do you mean that they're
being changed as part of the install process (e.g. .deb postinstall scripts), or
"files that can change after being installed"?  If the former, I'm confused;
wouldn't you already be running postinstall stuff before applying future
updates?

http://codereview.chromium.org/492008/diff/1/7#newcode45
src/platform/update_engine/delta_diff_generator.h:45: const std::set<string>&
always_full_paths,
Are you already pulling string out of the std namespace here?  Why does this
line omit the std:: while all of the other params use it?

A name like "nondiff_paths" or "paths_to_not_diff" may be more clear.

http://codereview.chromium.org/492008/diff/1/7#newcode80
src/platform/update_engine/delta_diff_generator.h:80: std::set<std::string>
always_full_new_paths,
s/new/target/ is maybe more clear

http://codereview.chromium.org/492008/diff/1/11
File src/platform/update_engine/generate_delta_main.cc (right):

http://codereview.chromium.org/492008/diff/1/11#newcode15
src/platform/update_engine/generate_delta_main.cc:15: #include
"update_engine/delta_diff_generator.h"
alphabetize these?

http://codereview.chromium.org/492008/diff/1/11#newcode34
src/platform/update_engine/generate_delta_main.cc:34: __attribute__ ((noreturn))
void usage(const char* argv0) {
any portability concerns with this?  (I think it's unlikely to catch any bugs
given the simplicity of the function, fwiw)

http://codereview.chromium.org/492008/diff/1/11#newcode50
src/platform/update_engine/generate_delta_main.cc:50: using namespace
chromeos_update_engine;
nit: pretty sure this is disallowed (vs. "using chromeos_update_engine::..." for
each name you want to include)

http://codereview.chromium.org/492008/diff/1/15
File src/platform/update_engine/main.cc (right):

http://codereview.chromium.org/492008/diff/1/15#newcode46
src/platform/update_engine/main.cc:46: };
DISALLOW_COPY_AND_ASSIGN()?

http://codereview.chromium.org/492008/diff/1/15#newcode57
src/platform/update_engine/main.cc:57: OmahaRequestPrepAction*
request_prep_action =
put these into shared_ptrs to begin with

http://codereview.chromium.org/492008/diff/1/15#newcode117
src/platform/update_engine/main.cc:117: Update(true);
Will this update trigger another full update if it fails, or am I missing
something that's present to prevent this?

http://codereview.chromium.org/492008/diff/1/19
File src/platform/update_engine/update_metadata.proto (right):

http://codereview.chromium.org/492008/diff/1/19#newcode106
src/platform/update_engine/update_metadata.proto:106: // unchanged from the old
version of the file
nit: add trailing period

[adlr, 2009-12-11 00:49:52]

http://codereview.chromium.org/492008/diff/1/6
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/492008/diff/1/6#newcode344
src/platform/update_engine/delta_diff_generator.cc:344: const std::string&
old_dir,
On 2009/12/10 23:29:10, Daniel Erat wrote:
> Why this instead of adding a "using std::string" to this file like you have
for
> other stuff?

i think it's b/c i copy/pasted the arg list from the .h file. I can take out hte
std::'s from the arg list here. what's your opinion?

http://codereview.chromium.org/492008/diff/1/6#newcode349
src/platform/update_engine/delta_diff_generator.cc:349: std::vector<char>* out,
On 2009/12/10 23:29:10, Daniel Erat wrote:
> You're already pulling vector out of std:: in this file.

same as previous comment

http://codereview.chromium.org/492008/diff/1/6#newcode369
src/platform/update_engine/delta_diff_generator.cc:369: vector<char> patch_file;
On 2009/12/10 23:29:10, Daniel Erat wrote:
> declare this down where you use it instead of way up here

The reason this is up here is because we may set ret (a pointer) to it. This is
confusing, so I changed the code to use swaps and have no confusing vector
pointers

http://codereview.chromium.org/492008/diff/1/6#newcode370
src/platform/update_engine/delta_diff_generator.cc:370: if (!avoid_diff) {
On 2009/12/10 23:29:10, Daniel Erat wrote:
> Since you're already returning early from a few places within this if
condition,
> I think it'd be cleaner to just have
> 
> if (!avoid_diff) {
>   out->swap(*ret);
>   return true;
> }
> 
> so you don't need extra indenting.

I assume you mean if (avoid_diff) {...

fixed

http://codereview.chromium.org/492008/diff/1/6#newcode376
src/platform/update_engine/delta_diff_generator.cc:376: *out = *ret;
On 2009/12/10 23:29:10, Daniel Erat wrote:
> swap()

Done.

http://codereview.chromium.org/492008/diff/1/6#newcode393
src/platform/update_engine/delta_diff_generator.cc:393: const string kPatchFile
= "/tmp/delta.patch";
On 2009/12/10 23:29:10, Daniel Erat wrote:
> use mkstemp() instead

mkstemp() opens the file. I need a temporary filename path that's unopened which
I can pass into bsdiff

http://codereview.chromium.org/492008/diff/1/6#newcode396
src/platform/update_engine/delta_diff_generator.cc:396:
cmd.push_back("/usr/bin/bsdiff");
On 2009/12/10 23:29:10, Daniel Erat wrote:
> move this path to the top of the file

Done.

http://codereview.chromium.org/492008/diff/1/6#newcode412
src/platform/update_engine/delta_diff_generator.cc:412: *out = *ret;
On 2009/12/10 23:29:10, Daniel Erat wrote:
> swap()

Done.

http://codereview.chromium.org/492008/diff/1/6#newcode457
src/platform/update_engine/delta_diff_generator.cc:457: // here with
always_full_paths expanded.
On 2009/12/10 23:29:10, Daniel Erat wrote:
> I think that this comment would be more useful above
WriteFileDiffsToDeltaFile()
> in the header.

Done.

http://codereview.chromium.org/492008/diff/1/7
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/492008/diff/1/7#newcode37
src/platform/update_engine/delta_diff_generator.h:37: // as a diff. This is
useful for files that change during postinstall, since
On 2009/12/10 23:29:10, Daniel Erat wrote:
> When you say "files that change during postinstall", do you mean that they're
> being changed as part of the install process (e.g. .deb postinstall scripts),
or
> "files that can change after being installed"?  If the former, I'm confused;
> wouldn't you already be running postinstall stuff before applying future
> updates?

The postinstall here doesn't refer to debian package postinstall. It refers to
files that are changed in the postinstall script that's run after applying an
update. The only example we have of this is /boot/extlinux.conf (the bootloader
configuration file).

http://codereview.chromium.org/492008/diff/1/7#newcode45
src/platform/update_engine/delta_diff_generator.h:45: const std::set<string>&
always_full_paths,
On 2009/12/10 23:29:10, Daniel Erat wrote:
> Are you already pulling string out of the std namespace here?  Why does this
> line omit the std:: while all of the other params use it?
> 
> A name like "nondiff_paths" or "paths_to_not_diff" may be more clear.

I am not pulling string out of std. I realized in a few .h file I did have using
directives. I fixed these files.

http://codereview.chromium.org/492008/diff/1/7#newcode80
src/platform/update_engine/delta_diff_generator.h:80: std::set<std::string>
always_full_new_paths,
On 2009/12/10 23:29:10, Daniel Erat wrote:
> s/new/target/ is maybe more clear

Done.

http://codereview.chromium.org/492008/diff/1/11
File src/platform/update_engine/generate_delta_main.cc (right):

http://codereview.chromium.org/492008/diff/1/11#newcode15
src/platform/update_engine/generate_delta_main.cc:15: #include
"update_engine/delta_diff_generator.h"
On 2009/12/10 23:29:10, Daniel Erat wrote:
> alphabetize these?

Done.

http://codereview.chromium.org/492008/diff/1/11#newcode34
src/platform/update_engine/generate_delta_main.cc:34: __attribute__ ((noreturn))
void usage(const char* argv0) {
On 2009/12/10 23:29:10, Daniel Erat wrote:
> any portability concerns with this?  (I think it's unlikely to catch any bugs
> given the simplicity of the function, fwiw)

okay, took it out

http://codereview.chromium.org/492008/diff/1/11#newcode50
src/platform/update_engine/generate_delta_main.cc:50: using namespace
chromeos_update_engine;
On 2009/12/10 23:29:10, Daniel Erat wrote:
> nit: pretty sure this is disallowed (vs. "using chromeos_update_engine::..."
for
> each name you want to include) 

Done.

http://codereview.chromium.org/492008/diff/1/15
File src/platform/update_engine/main.cc (right):

http://codereview.chromium.org/492008/diff/1/15#newcode46
src/platform/update_engine/main.cc:46: };
On 2009/12/10 23:29:10, Daniel Erat wrote:
> DISALLOW_COPY_AND_ASSIGN()?

Done.

http://codereview.chromium.org/492008/diff/1/15#newcode57
src/platform/update_engine/main.cc:57: OmahaRequestPrepAction*
request_prep_action =
On 2009/12/10 23:29:10, Daniel Erat wrote:
> put these into shared_ptrs to begin with

Done.

http://codereview.chromium.org/492008/diff/1/15#newcode117
src/platform/update_engine/main.cc:117: Update(true);
On 2009/12/10 23:29:10, Daniel Erat wrote:
> Will this update trigger another full update if it fails, or am I missing
> something that's present to prevent this?

good catch. I need to test this code.

The idea is we first try a delta update. If processing is done, and we didn't
succeed, and we didn't do a full update, try again with a full update.

http://codereview.chromium.org/492008/diff/1/19
File src/platform/update_engine/update_metadata.proto (right):

http://codereview.chromium.org/492008/diff/1/19#newcode106
src/platform/update_engine/update_metadata.proto:106: // unchanged from the old
version of the file
On 2009/12/10 23:29:10, Daniel Erat wrote:
> nit: add trailing period

Done.

[Daniel Erat, 2009-12-11 01:00:17]

http://codereview.chromium.org/492008/diff/1/6
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/492008/diff/1/6#newcode344
src/platform/update_engine/delta_diff_generator.cc:344: const std::string&
old_dir,
On 2009/12/11 00:49:52, adlr wrote:
> On 2009/12/10 23:29:10, Daniel Erat wrote:
> > Why this instead of adding a "using std::string" to this file like you have
> for
> > other stuff?
> 
> i think it's b/c i copy/pasted the arg list from the .h file. I can take out
hte
> std::'s from the arg list here. what's your opinion?

Yeah, makes sense to omit them here since you can.

http://codereview.chromium.org/492008/diff/1/6#newcode393
src/platform/update_engine/delta_diff_generator.cc:393: const string kPatchFile
= "/tmp/delta.patch";
On 2009/12/11 00:49:52, adlr wrote:
> On 2009/12/10 23:29:10, Daniel Erat wrote:
> > use mkstemp() instead
> 
> mkstemp() opens the file. I need a temporary filename path that's unopened
which
> I can pass into bsdiff

Close it before calling bsdiff?  The copy of bsdiff on my workstation seems to
be fine with the output file already existing.

http://codereview.chromium.org/492008/diff/1/7
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/492008/diff/1/7#newcode37
src/platform/update_engine/delta_diff_generator.h:37: // as a diff. This is
useful for files that change during postinstall, since
On 2009/12/11 00:49:52, adlr wrote:
> On 2009/12/10 23:29:10, Daniel Erat wrote:
> > When you say "files that change during postinstall", do you mean that
they're
> > being changed as part of the install process (e.g. .deb postinstall
scripts),
> or
> > "files that can change after being installed"?  If the former, I'm confused;
> > wouldn't you already be running postinstall stuff before applying future
> > updates?
> 
> The postinstall here doesn't refer to debian package postinstall. It refers to
> files that are changed in the postinstall script that's run after applying an
> update. The only example we have of this is /boot/extlinux.conf (the
bootloader
> configuration file).

Okay.  Do we skip running this postinstall script when applying consecutive
updates, or defer it until after reboot or something?

http://codereview.chromium.org/492008/diff/1/7#newcode45
src/platform/update_engine/delta_diff_generator.h:45: const std::set<string>&
always_full_paths,
On 2009/12/11 00:49:52, adlr wrote:
> On 2009/12/10 23:29:10, Daniel Erat wrote:
> > Are you already pulling string out of the std namespace here?  Why does this
> > line omit the std:: while all of the other params use it?
> > 
> > A name like "nondiff_paths" or "paths_to_not_diff" may be more clear.
> 
> I am not pulling string out of std. I realized in a few .h file I did have
using
> directives. I fixed these files.

Thanks (also made a comment about "full" being kinda vague here that you
might've missed; okay to leave it if you think it's clear though).

[adlr, 2009-12-11 01:20:10]

Thanks for the quick review and comments.

http://codereview.chromium.org/492008/diff/1/6
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/492008/diff/1/6#newcode344
src/platform/update_engine/delta_diff_generator.cc:344: const std::string&
old_dir,
On 2009/12/11 01:00:17, Daniel Erat wrote:
> On 2009/12/11 00:49:52, adlr wrote:
> > On 2009/12/10 23:29:10, Daniel Erat wrote:
> > > Why this instead of adding a "using std::string" to this file like you
have
> > for
> > > other stuff?
> > 
> > i think it's b/c i copy/pasted the arg list from the .h file. I can take out
> hte
> > std::'s from the arg list here. what's your opinion?
> 
> Yeah, makes sense to omit them here since you can.

Done.

http://codereview.chromium.org/492008/diff/1/6#newcode393
src/platform/update_engine/delta_diff_generator.cc:393: const string kPatchFile
= "/tmp/delta.patch";
On 2009/12/11 01:00:17, Daniel Erat wrote:
> On 2009/12/11 00:49:52, adlr wrote:
> > On 2009/12/10 23:29:10, Daniel Erat wrote:
> > > use mkstemp() instead
> > 
> > mkstemp() opens the file. I need a temporary filename path that's unopened
> which
> > I can pass into bsdiff
> 
> Close it before calling bsdiff?  The copy of bsdiff on my workstation seems to
> be fine with the output file already existing.

what I mean is, mkstemp doesn't tell me the filename of the file it creates, so
how can I pass that file name to bsidff?

http://codereview.chromium.org/492008/diff/1/7
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/492008/diff/1/7#newcode37
src/platform/update_engine/delta_diff_generator.h:37: // as a diff. This is
useful for files that change during postinstall, since
On 2009/12/11 01:00:17, Daniel Erat wrote:
> On 2009/12/11 00:49:52, adlr wrote:
> > On 2009/12/10 23:29:10, Daniel Erat wrote:
> > > When you say "files that change during postinstall", do you mean that
> they're
> > > being changed as part of the install process (e.g. .deb postinstall
> scripts),
> > or
> > > "files that can change after being installed"?  If the former, I'm
confused;
> > > wouldn't you already be running postinstall stuff before applying future
> > > updates?
> > 
> > The postinstall here doesn't refer to debian package postinstall. It refers
to
> > files that are changed in the postinstall script that's run after applying
an
> > update. The only example we have of this is /boot/extlinux.conf (the
> bootloader
> > configuration file).
> 
> Okay.  Do we skip running this postinstall script when applying consecutive
> updates, or defer it until after reboot or something?

We run postinst after installing each update (before reboot). So for consecutive
updates w/o a reboot between, we would run the postinst for each update.

http://codereview.chromium.org/492008/diff/1/7#newcode45
src/platform/update_engine/delta_diff_generator.h:45: const std::set<string>&
always_full_paths,
On 2009/12/11 01:00:17, Daniel Erat wrote:
> On 2009/12/11 00:49:52, adlr wrote:
> > On 2009/12/10 23:29:10, Daniel Erat wrote:
> > > Are you already pulling string out of the std namespace here?  Why does
this
> > > line omit the std:: while all of the other params use it?
> > > 
> > > A name like "nondiff_paths" or "paths_to_not_diff" may be more clear.
> > 
> > I am not pulling string out of std. I realized in a few .h file I did have
> using
> > directives. I fixed these files.
> 
> Thanks (also made a comment about "full" being kinda vague here that you
> might've missed; okay to leave it if you think it's clear though).

oops, missed it. fixed

[Daniel Erat, 2009-12-11 01:30:13]

http://codereview.chromium.org/492008/diff/1/6
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/492008/diff/1/6#newcode393
src/platform/update_engine/delta_diff_generator.cc:393: const string kPatchFile
= "/tmp/delta.patch";
On 2009/12/11 01:20:10, adlr wrote:
> On 2009/12/11 01:00:17, Daniel Erat wrote:
> > On 2009/12/11 00:49:52, adlr wrote:
> > > On 2009/12/10 23:29:10, Daniel Erat wrote:
> > > > use mkstemp() instead
> > > 
> > > mkstemp() opens the file. I need a temporary filename path that's unopened
> > which
> > > I can pass into bsdiff
> > 
> > Close it before calling bsdiff?  The copy of bsdiff on my workstation seems
to
> > be fine with the output file already existing.
> 
> what I mean is, mkstemp doesn't tell me the filename of the file it creates,
so
> how can I pass that file name to bsidff?

mkstemp() updates the suffix of its 'template' parameter; you can just get it
from there.

http://codereview.chromium.org/492008/diff/1/7
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/492008/diff/1/7#newcode37
src/platform/update_engine/delta_diff_generator.h:37: // as a diff. This is
useful for files that change during postinstall, since
On 2009/12/11 01:20:10, adlr wrote:
> On 2009/12/11 01:00:17, Daniel Erat wrote:
> > On 2009/12/11 00:49:52, adlr wrote:
> > > On 2009/12/10 23:29:10, Daniel Erat wrote:
> > > > When you say "files that change during postinstall", do you mean that
> > they're
> > > > being changed as part of the install process (e.g. .deb postinstall
> > scripts),
> > > or
> > > > "files that can change after being installed"?  If the former, I'm
> confused;
> > > > wouldn't you already be running postinstall stuff before applying future
> > > > updates?
> > > 
> > > The postinstall here doesn't refer to debian package postinstall. It
refers
> to
> > > files that are changed in the postinstall script that's run after applying
> an
> > > update. The only example we have of this is /boot/extlinux.conf (the
> > bootloader
> > > configuration file).
> > 
> > Okay.  Do we skip running this postinstall script when applying consecutive
> > updates, or defer it until after reboot or something?
> 
> We run postinst after installing each update (before reboot). So for
consecutive
> updates w/o a reboot between, we would run the postinst for each update.

Okay, so these are (sort of) mutable files that live in the updated partition,
and we need to update them but can't depend on their previous state on-disk. 
Can this be simplified by omitting them entirely from the update and writing
them from scratch in the postinstall script, or does that cause other issues
that I'm not thinking of?

[adlr, 2009-12-11 01:41:29]

http://codereview.chromium.org/492008/diff/1/6
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/492008/diff/1/6#newcode393
src/platform/update_engine/delta_diff_generator.cc:393: const string kPatchFile
= "/tmp/delta.patch";
On 2009/12/11 01:30:13, Daniel Erat wrote:
> On 2009/12/11 01:20:10, adlr wrote:
> > On 2009/12/11 01:00:17, Daniel Erat wrote:
> > > On 2009/12/11 00:49:52, adlr wrote:
> > > > On 2009/12/10 23:29:10, Daniel Erat wrote:
> > > > > use mkstemp() instead
> > > > 
> > > > mkstemp() opens the file. I need a temporary filename path that's
unopened
> > > which
> > > > I can pass into bsdiff
> > > 
> > > Close it before calling bsdiff?  The copy of bsdiff on my workstation
seems
> to
> > > be fine with the output file already existing.
> > 
> > what I mean is, mkstemp doesn't tell me the filename of the file it creates,
> so
> > how can I pass that file name to bsidff?
> 
> mkstemp() updates the suffix of its 'template' parameter; you can just get it
> from there.

Oh, I never noticed that property of mkstemp. Changed to use mkstemp

http://codereview.chromium.org/492008/diff/1/7
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/492008/diff/1/7#newcode37
src/platform/update_engine/delta_diff_generator.h:37: // as a diff. This is
useful for files that change during postinstall, since
On 2009/12/11 01:30:13, Daniel Erat wrote:
> On 2009/12/11 01:20:10, adlr wrote:
> > On 2009/12/11 01:00:17, Daniel Erat wrote:
> > > On 2009/12/11 00:49:52, adlr wrote:
> > > > On 2009/12/10 23:29:10, Daniel Erat wrote:
> > > > > When you say "files that change during postinstall", do you mean that
> > > they're
> > > > > being changed as part of the install process (e.g. .deb postinstall
> > > scripts),
> > > > or
> > > > > "files that can change after being installed"?  If the former, I'm
> > confused;
> > > > > wouldn't you already be running postinstall stuff before applying
future
> > > > > updates?
> > > > 
> > > > The postinstall here doesn't refer to debian package postinstall. It
> refers
> > to
> > > > files that are changed in the postinstall script that's run after
applying
> > an
> > > > update. The only example we have of this is /boot/extlinux.conf (the
> > > bootloader
> > > > configuration file).
> > > 
> > > Okay.  Do we skip running this postinstall script when applying
consecutive
> > > updates, or defer it until after reboot or something?
> > 
> > We run postinst after installing each update (before reboot). So for
> consecutive
> > updates w/o a reboot between, we would run the postinst for each update.
> 
> Okay, so these are (sort of) mutable files that live in the updated partition,
> and we need to update them but can't depend on their previous state on-disk. 
> Can this be simplified by omitting them entirely from the update and writing
> them from scratch in the postinstall script, or does that cause other issues
> that I'm not thinking of?

Your understanding is correct. We could write those files out completely in the
postinst script, but that would complicate the build process.

Plus, long term we aren't going to modify any files on the installed filesystem,
so this will go away.