1059567 - Packaging changes for the move of removed-files file from Contents/MacOS to Contents/Resources

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

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 1

[Robert Strong (they/them - no direct email)]

Attachment: Mar packaging code for removed-files (wip)

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: Mar packaging code for removed-files (wip)

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: Firefox removed-files.in patch

Comment 4

[Robert Strong (they/them - no direct email)]

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 5

[Robert Strong (they/them - no direct email)]

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 6

[Benjamin Smedberg]

Comment on attachment 8481035 [details] [diff] [review]
Firefox removed-files.in patch

I don't see a problem with this.

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: Mar packaging code for removed-files (wip)

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

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

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!

Comment 9

[Robert Strong (they/them - no direct email)]

(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? ;)

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: Mar packaging code for removed-files (wip)

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: Mar packaging code for removed-files

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 12

[Robert Strong (they/them - no direct email)]

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.

Comment 13

[Robert Strong (they/them - no direct email)]

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?

Comment 14

[Nick Thomas [:nthomas] (UTC+12)]

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 15

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 16

[Robert Strong (they/them - no direct email)]

(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.

Comment 17

[Robert Strong (they/them - no direct email)]

(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

Comment 18

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 19

[Nick Thomas [:nthomas] (UTC+12)]

(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 ?

Comment 20

[Robert Strong (they/them - no direct email)]

(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

Comment 21

[Robert Strong (they/them - no direct email)]

(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.

Comment 22

[Robert Strong (they/them - no direct email)]

(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

Comment 23

[Robert Strong (they/them - no direct email)]

Attachment: Mar packaging code for removed-files rev2

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.

Comment 24

[Nick Thomas [:nthomas] (UTC+12)]

(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

Comment 25

[Robert Strong (they/them - no direct email)]

Attachment: Combined patch

Comment 26

[Robert Strong (they/them - no direct email)]

Attachment: Combined patch

Unbitrotted

Comment 27

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/7abb157a86c4

Comment 29

[Robert Strong (they/them - no direct email)]

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

Comment 31

[Robert Strong (they/them - no direct email)]

Attachment: Combined patch

Includes the fix from bug 1070811

Comment 32

[Robert Strong (they/them - no direct email)]

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

Comment 33

[Robert Strong (they/them - no direct email)]

(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.

Comment 34

[Robert Strong (they/them - no direct email)]

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

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/432ef1ab72b0

Comment 36

[Robert Strong (they/them - no direct email)]

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