This bug is for the self-contained executable for the Firefox 1.1 update plan. It needs to be small, support binary patching, etc. http://wiki.mozilla.org/Software_Update http://wiki.mozilla.org/Firefox:1.1_Software_Update_Upgrades I am attaching what I've got here. It builds and runs on mac. The binary patching code is untested, because I (or someone) needs to update "bsdiff" to add the checksum and remove bzip2, and make everything 32bit. But it's a start, and it's small enough to make me happy.

The updater/ bits of this patch build on mac, and appear to basically work. The other-licenses/bsdiff parts do not build. The first priority here is to get bsdiff working with the same header/32bit types as bspatch. Since I'm going to be having a new baby tomorrow, and this is fairly urgent, I welcome assistance with that task. If you do start on something, please comment to let me know, so that we don't duplicate activity.

FYI, I'm working on an revised version of this patch while bsmedberg is on vacation. For the most part, I'm very happy with the current patch. I'm adding the following pieces: * Recover better from errors from Execute steps: 1) Add Finish step to each Action after all Execute steps have run. 2) Make backup copies of all files before removing any files. 3) Restore backups in Finish step if any Execute step failed. * Implement UI. For this task, my plan is as follows: 1) Move realmain onto a background thread. 2) Record state of realmain in some global progress variables. 3) Protect global progress variables in a mutex. 4) Implement native progress UI that polls progress variables. I also had to remove the submanifest action because I don't have an implementation of fork, though I could probably spawn a thread for that given the architecture described above. I'm just not sure that there is any need for a submanifest action at this point.

> 1) Move realmain onto a background thread. Actually, nevermind that. I'm going start off by avoiding any multithreading. That's probably overkill for this application. We should have the opportunity to update the UI often enough such that this sort of thing would be unnecessary. Another TODO item is adding code to the updater to make it wait for its parent process to exit. That way we can be sure that firefox.exe is not running when we start patching it. I'm thinking of passing the process ID of the parent as a command line flag to the updater.

> Another TODO item is adding code to the updater to make it wait for its parent > process to exit. That way we can be sure that firefox.exe is not running when For *nix/windows this is not necessary... we can just execv Firefox into the update executable. On mac this will require more testing, and perhaps the parent-waiting code you describe.

> For *nix/windows this is not necessary... we can just execv Firefox into the > update executable. On mac this will require more testing, and perhaps the > parent-waiting code you describe. Ah, yeah.. that does make sense. By the way, I have windows progress UI working. I did end up putting realmain on a background thread for simplicity -- yeah, it made things simpler. The resulting updater.exe is less than 20k in an optimized build.

OK, here's a revised version of the updater. It has UI for Windows and GTK2. I've only verified the ADD function. I verified that I was able to implement "cp -r" using this tool ;-) Remaining work: 1) Support re-launching firefox with command line flag 2) Add OSX progress UI 3) Test patching and file removal 4) Make the progress increments make sense. Right now, I slice it into thirds: 1/3 for Prepare, 1/3 for Execute, and 1/3 for Finish, but Execute dominates in terms of time, so the progress meter progresses in a funny way.

> For *nix/windows this is not necessary... we can just execv Firefox into the > update executable. On mac this will require more testing, and perhaps the > parent-waiting code you describe. So, I think that MS's execv might not work the way we think or hope that it does. It appears to call CreateProcess and then call _exit(0) on the process that called execv. Therefore, I think there would be a race between the new process's WinMain and the call to _exit(0), which I presume is what would release "firefox.exe" to allow it to be overwritten. It may be difficult to lose this race in practice, but it might bite us if we aren't careful. Under UNIX, it would be nice to use execv because it allows people running firefox-bin to wait on the process and have that correctly correspond to waiting for firefox "the application" to close. But, that is not really needed on Windows, so if we need to do the --wait-pid=PID thing on Windows, then so be it.

Agreed. By the way, I plan to examine why we had to switch from execv to the MacLaunchHelper code in the first place... I think that switching back to execv on the mac (with the appropriate problems fixed) could fix a number of annoying mac bugs (the disappearing/reapparing bouncy globe, among others).

This patch is the latest from my tree. Not sure when bsmedberg may be back to start thinking about this stuff again, so I wanted to get my latest patch up here. I've revised the updater UI to only appear if the update is taking a while to process. In other words, for very short updates, we won't show any UI at this level. The UI will be shown by the update service if needed. I've used this code to apply a complete firefox ZIP archive into a directory. This probably only compiles on Windows. I need to spend some time on Linux and OSX making things happy there.

This looks decent to me. I'm interested in getting something checked in on the software update branch soon, so that I can play. We also talked about not extracting the ZIP files, and letting this executable extract them as it uses them: this means linking against standalone libjar, but that's not a big hit.

Yeah, I'm close to having the zipreader stuff incorporated. Not bad so far at least. I'll land this on the branch once I have that in shape.

This verison of the updater works on Windows, Linux, and Mac OSX. Mac OSX is missing platform specific progress meter UI. Josh Aas is working on that. For now, we just perform the update completely silently on Mac OSX (and any other platform for that matter). This patch depends on libmar and libbz2.

Comment on attachment 185092 [details] [diff] [review] v1 patch 1) in the GTK AutoFILE impl, you need to null-check fp_ in the destructor: fclose(0) will crash on some OSes (BSD). 2) in main(), the usage statement should not be #ifdef DEBUG 3) why don't we wait-pid on mac also? We can't use execv there because of the threading problems so I think we have to use waitpid. 4) Do we have a localization story for updater.ini? It is hardcoding "Firefox". I think this should be in browser/locales and mail/locales, perhaps with a hardcoded generic English text if the ini file is missing.

> 1) in the GTK AutoFILE impl, you need to null-check fp_ in the destructor: > fclose(0) will crash on some OSes (BSD). ok > 2) in main(), the usage statement should not be #ifdef DEBUG ok... i didn't see any reason to output it in optimized builds. but, i guess someone might want to run it from the command line. > 3) why don't we wait-pid on mac also? We can't use execv there because of the > threading problems so I think we have to use waitpid. ok, sounds reasonable. > 4) Do we have a localization story for updater.ini? I figured localizing this file would be similar to how we localize the INI files for the installers. Why is that not okay?

We localize the installer.ini files from a makefile in browser/locales (which calls into toolkit/locales). I really think we should ship this file from that directory.

bsmedberg: can you please outline what needs to be done exactly to localize updater.ini? i sort of get what's going on, but it looks like i need to do some preprocessing steps and call iconv as well. what's the recommended approach exactly? can you help with this task?

> 3) why don't we wait-pid on mac also? We can't use execv there because of the > threading problems so I think we have to use waitpid. what threading problem? clue me in :)

What charset is the ini file? it looks like utf8 on gtk2 and the "platform" charset on windows. I'm willing to ignore that issue to get this landed, but we're going to need to fix it. I suggest that we just put the ini file in browser/locales/en-US/update.ini and ship it with the same rule we use for README.txt for the time being. The reason we use the MacLaunhHelper on mac is because execv fails once the mac windowing system has been initialized: execv apparently doesn't work on mac if there are non-main threads still running, and the windowing system uses a background thread to process events.

bsmedberg: sounds good. i will make those changes.

On the topic of charsets, you are right. If we assume that localizers are creating updater.ini using UTF-8, then we need to run it through iconv to convert to the platform charset before shipping it with a localized Windows build of Firefox. I guess that means using the same system that we are using for installer.inc currently.

Right now iconv is only a build-time requirement for the installer: we will need to update the build docs and make the proper noise on the newsgroups and blogs that iconv is now a build-time requirement for anyone building toolkit apps.

it would be great if we could use unicode throughout these apps and use MSLU...

Perhaps I should create an --enable-updater option to configure. That will probably be appreciated by folks who either use Mozilla on a platform where the updater doesn't build or by folks who simply don't want to build the updater (hello Minimo!).

OK, this version implements --enable-updater. It moves updater.ini into browser/locales/en-US, and adds a rule to browser/locale/Makefile.in to run updater.ini through iconv. It only does this if MOZ_UPDATER is defined. This patch also contains changes to client.mk, Makefile.in, and allmakefiles.sh to build the various dependencies of the updater system: libmar, libbz2, and bsdiff.

The last patch was missing configure.in changes. FYI, this entire patch is checked in on the SOFTWARE_UPDATE_20050428_BRANCH.

Comment on attachment 185541 [details] [diff] [review] v2.1 patch In configure.in, you need to set a default value for MOZ_UPDATER: I think it should be on-by-default for MOZ_XUL_APP. Be careful not to checkin the client.mk changes. In Makefile.in, it should be "Cross-compiling the mbsdiff executable is not yet supported." But, I don't think we should compile it by default at all: it's a tainted license, and should require an opt-in build rule.

OK, revised patch.

Comment on attachment 185623 [details] [diff] [review] v2.2 patch Marking approvals based on previous discussions.

I've modified updater/Makefile.in to only build progressui_gtk.cpp when MOZ_ENABLE_GTK2 is defined, because that code makes use of several GTK2 specific functions, including the new and much saner progressbar API. If someone wants to port it to GTK1 they can. For now, GTK1 builds will just use progressui_null.

fixed-on-trunk

I'm getting this compilation error (with VS8 Beta2) updater.cpp c:/mozilla/trunk/mozilla/toolkit/mozapps/update/src/updater/updater.cpp(240) : e rror C2440: 'initializing' : cannot convert from 'const char *' to 'char *' Conversion loses qualifiers c:/mozilla/trunk/mozilla/toolkit/mozapps/update/src/updater/updater.cpp(465) : e rror C2440: 'initializing' : cannot convert from 'const char *' to 'char *' Conversion loses qualifiers make[7]: *** [updater.obj] Error 2

I checked in a fix for the VC8 Beta 2 build bustage.

*** Bug 297128 has been marked as a duplicate of this bug. ***

(In reply to comment #21) > Right now iconv is only a build-time requirement for the installer: we will need > to update the build docs and make the proper noise on the newsgroups and blogs > that iconv is now a build-time requirement for anyone building toolkit apps. I ran into this today on win32 as well. You should add a configure check for this since it's a new mandatory requirement.

cls: --disable-updater should bypass the iconv requirement.

Oh,... you're saying that we should make configure complain if --disable-updater is not specified and iconv cannot be found. That does sound like a good idea.

Breaks BeOS-builds as it doesn't have implementation.

Please do not reopen this bug for platform specific work like this. File a new bug. Unsupported platforms should be compiled with --disable-updater.

Sorry, there was no way of knowing that. (It might be good to note things like that along with the error that stops the build. Saves everyone a lot of trouble.)

Yeah, sorry about that. I'm trying to move quickly, and as a result communication about --disable-updater hasn't been so good.