On Mac OS the removed-files file will need to be moved to Contents/Resources and changes to it will need to be included in partial mar files. Since shared Mozilla files in the Mac bundle can now be located under directories other than Contents/MacOS with the v2 signing changes the removed-files.in file will need to be able to know these new locations.

Comment on attachment 8481035 [details] [diff] [review] Firefox removed-files.in patch Benjamin, can I get your feedback for this approach? The removed-files file is seldom needed anymore so I doubt people will need to deal with the defines often.

Comment on attachment 8481034 [details] [diff] [review] Mar packaging code for removed-files (wip) Nick, I had to make a copy of the removed-files file to it wouldn't be in use. Let me know what you think of this approach. Thanks!

Comment on attachment 8481035 [details] [diff] [review] Firefox removed-files.in patch I don't see a problem with this.

Found a problem with platform detection that made the tests fail so I went with what we use elsewhere.

Comment on attachment 8481645 [details] [diff] [review] Mar packaging code for removed-files (wip) Review of attachment 8481645 [details] [diff] [review]: ----------------------------------------------------------------- Heading in the right direction. ::: tools/update-packaging/make_full_update.sh @@ +62,5 @@ > fi > fi > > +if [ -f "removed-files" ]; then > + cp "removed-files" "removed-files.tmp" I didn't manage to grok why the copy was needed. append_remove_instructions() doesn't seem to be using the copy, and I don't recall any parallelism that might lead to access conflicts. What situations were you having issues in ? ::: tools/update-packaging/make_incremental_updates.py @@ +319,5 @@ > for line in lines: > # Exclude any blank and comment lines. > if line and not line.startswith("#"): > + line = line.replace("@MACOS@", macos) > + line = line.replace("@RESOURCES@", resources) Preprocessor has already done this during make package ? @@ +426,2 @@ > """ extracts buildid from MAR > TODO: this should handle 1.8 branch too Hah!

(In reply to Nick Thomas [:nthomas] from comment #8) > Comment on attachment 8481645 [details] [diff] [review] > Mar packaging code for removed-files (wip) > > Review of attachment 8481645 [details] [diff] [review]: > ----------------------------------------------------------------- > > Heading in the right direction. > > ::: tools/update-packaging/make_full_update.sh > @@ +62,5 @@ > > fi > > fi > > > > +if [ -f "removed-files" ]; then > > + cp "removed-files" "removed-files.tmp" > > I didn't manage to grok why the copy was needed. > append_remove_instructions() doesn't seem to be using the copy, and I don't > recall any parallelism that might lead to access conflicts. What situations > were you having issues in ? Before this patch partials excluded the removed-files file and I was getting a file in use error when generating partials. I'll investigate further if I have time. > ::: tools/update-packaging/make_incremental_updates.py > @@ +319,5 @@ > > for line in lines: > > # Exclude any blank and comment lines. > > if line and not line.startswith("#"): > > + line = line.replace("@MACOS@", macos) > > + line = line.replace("@RESOURCES@", resources) > > Preprocessor has already done this during make package ? Yes. I thought I had resubmitted the updated patch. :( > > @@ +426,2 @@ > > """ extracts buildid from MAR > > TODO: this should handle 1.8 branch too > > Hah! Want me to add this? ;)

Nick, there are file in use errors when running the mar packaging tests on Windows so I left the .tmp file instead of spending more time on this. If this is of concern perhaps a followup?

Comment on attachment 8481035 [details] [diff] [review] Firefox removed-files.in patch The test changes to support v2 signing will be done in bug bug1058182.

Nick, a couple of concerns 1. file exclusions since if it is in the signed dmg it should be in the update as well. The only exclusion I see is readme.txt and that is only excluded by partials. 2. if the channel-prefs.js or the update-settings.ini files are different between the old dmg and the new dmg then the update will break the signature. Any ideas of where such a check should be performed?

re 1, I can't actually find a readme.txt in recent nightly or release, may well be vestigial from the days of talkback. re 2, haven't wrapped my head around this, could you add more detail ?

Comment on attachment 8486985 [details] [diff] [review] Mar packaging code for removed-files Review of attachment 8486985 [details] [diff] [review]: ----------------------------------------------------------------- r+ based on the earlier feedback actually being a review level check, and looking at the interdiff. The need for removed-files.tmp still worries me a little because the copy isn't used, and may be a symptom of some other bug - eg something is holding a file open, and adding the .tmp just moves that elsewhere.

(In reply to Nick Thomas [:nthomas] from comment #14) > re 1, I can't actually find a readme.txt in recent nightly or release, may > well be vestigial from the days of talkback. Right and if another sneaks into the dmg then after an update the signature could be invalid. > re 2, haven't wrapped my head around this, could you add more detail ? If either of those files change in any way (editing, preprocessing, etc.) then the signature would be invalid.

(In reply to Nick Thomas [:nthomas] from comment #15) > Comment on attachment 8486985 [details] [diff] [review] > Mar packaging code for removed-files > > Review of attachment 8486985 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ based on the earlier feedback actually being a review level check, and > looking at the interdiff. The need for removed-files.tmp still worries me a > little because the copy isn't used, and may be a symptom of some other bug - > eg something is holding a file open, and adding the .tmp just moves that > elsewhere. Previously, the removed-files file was not included in partial updates so this wasn't seen before since it only affects the python script which only makes partials. I think it has to do with the call to bz2.BZ2File

Might be fixed by this then: diff --git a/tools/update-packaging/make_incremental_updates.py b/tools/update-packaging/make_incremental_updates.py --- a/tools/update-packaging/make_incremental_updates.py +++ b/tools/update-packaging/make_incremental_updates.py @@ -309,12 +309,13 @@ def process_explicit_remove_files(dir_pa if (os.path.exists(list_file_path)): list_file = bz2.BZ2File(list_file_path,"r") # throws if doesn't exist lines = [] for line in list_file: lines.append(line.strip()) + list_file.close() Otherwise python will garbage collect at some point, or cleanup on exit.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #16) > > re 2, haven't wrapped my head around this, could you add more detail ? > If either of those files change in any way (editing, preprocessing, etc.) > then the signature would be invalid. bhearsum knows a lot more about this work than I but the possibilities seem to be * the user has modified/removed the files, sig is invalid already. If they can update we'll add them and supply the new sig. sig and files are now consistent * we change the files, eg new info in update-settings.ini or something. Both types of update will add-if-not which ends up doing add. Sig is generated using the same files, so it's all consistent after the update That seems to easy, so I'm missing something right ?

(In reply to Nick Thomas [:nthomas] from comment #18) >... > + list_file.close() > > Otherwise python will garbage collect at some point, or cleanup on exit. Will try that out

(In reply to Nick Thomas [:nthomas] from comment #19) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #16) > > > re 2, haven't wrapped my head around this, could you add more detail ? > > If either of those files change in any way (editing, preprocessing, etc.) > > then the signature would be invalid. > > bhearsum knows a lot more about this work than I but the possibilities seem > to be > * the user has modified/removed the files, sig is invalid already. If they > can update we'll add them and supply the new sig. sig and files are now > consistent > * we change the files, eg new info in update-settings.ini or something. Both > types of update will add-if-not which ends up doing add. Sig is generated > using the same files, so it's all consistent after the update > > That seems to easy, so I'm missing something right ? The first case we can't control (other apps can't either) so skip that. For the second case what I am saying is that if anything on our end modifies either of these files the signature will be broken. The add-if-not instruction only adds if the file is missing in the destination so it won't fix it. What we would need to do is verify that the file hasn't changed. So, it should be possible to check if it has changed when generating the partial which would need to happen before the complete is made available. If the check fails no update is published, etc. Another option would be to always add them but that would break the update on beta to release without changing the channel. I am less concerned about the manual change that is done to test updates since an additional manual step can be performed to put it back if desired.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #20) > (In reply to Nick Thomas [:nthomas] from comment #18) > >... > > + list_file.close() > > > > Otherwise python will garbage collect at some point, or cleanup on exit. > Will try that out There is some weird interaction going on here. What I am seeing is removed-files can either be read or it can be included in the patch but if both are done even with the close statement the removed-files file will be in use and the call to shutil.rmtree(work_dir_root) will fail

Nick, doh! Figured it out. Now that the file is included in partial patches it is unzipped by the time that it is read. I also removed the exclusion of the readme file per comment #16.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #21) > For the second case what I am saying is that if anything on our end modifies > either of these files the signature will be broken. The add-if-not > instruction only adds if the file is missing in the destination so it won't > fix it. What we would need to do is verify that the file hasn't changed. So, > it should be possible to check if it has changed when generating the partial > which would need to happen before the complete is made available. If the > check fails no update is published, etc. Another option would be to always > add them but that would break the update on beta to release without changing > the channel. [snip] I see what you mean, urgh. In both nightly/aurora and beta/release cases the automation won't upload a complete if the partial fails (aka make upload call follows all packaging/update creation). That may change with projects https://wiki.mozilla.org/User:Ffledgling/Senbonzakura, and hard to enforce over time too. We don't strictly need to push release builds to beta users on mac, but would be great to avoid platform differences if we can. Another issue is the preprocessor leaving a comment at the top of channel-prefs.js, eg //@line 2 "/builds/slave/rel-m-beta-osx64-bld/build/browser/app/profile/channel-prefs.js" If we can suppress that implementation detail that would help avoid spurious check failures. At first glance python/mozbuild/mozbuild/preprocessor.py doesn't support that. :-S

Unbitrotted

For some reason the paths aren't getting fixed up. :(

Includes the fix from bug 1070811

Landed updated patch on oak https://hg.mozilla.org/projects/oak/rev/62b66973acac

(In reply to Nick Thomas [:nthomas] from comment #24) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #21) > > For the second case what I am saying is that if anything on our end modifies > > either of these files the signature will be broken. The add-if-not > > instruction only adds if the file is missing in the destination so it won't > > fix it. What we would need to do is verify that the file hasn't changed. So, > > it should be possible to check if it has changed when generating the partial > > which would need to happen before the complete is made available. If the > > check fails no update is published, etc. Another option would be to always > > add them but that would break the update on beta to release without changing > > the channel. [snip] > > I see what you mean, urgh. In both nightly/aurora and beta/release cases the > automation won't upload a complete if the partial fails (aka make upload > call follows all packaging/update creation). That may change with projects > https://wiki.mozilla.org/User:Ffledgling/Senbonzakura, and hard to enforce > over time too. > > We don't strictly need to push release builds to beta users on mac, but > would be great to avoid platform differences if we can. > > Another issue is the preprocessor leaving a comment at the top of > channel-prefs.js, eg > //@line 2 > "/builds/slave/rel-m-beta-osx64-bld/build/browser/app/profile/channel-prefs. > js" > If we can suppress that implementation detail that would help avoid spurious > check failures. At first glance python/mozbuild/mozbuild/preprocessor.py > doesn't support that. :-S Fixed in bug 1070863 I'll file a bug for checking that the channel-prefs.js and update-settings.ini files are the same as the previous build.

Pushed to fx-team https://hg.mozilla.org/integration/fx-team/rev/432ef1ab72b0

Landed on aurora in the Mac V2 signing combined patch in bug 1047584