Closed Bug 1418425 Opened 7 years ago Closed 7 years ago

Remove bz2 and use the zlib crc32 function instead

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox62 fixed)

Status:
VERIFIED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

Bug 1418425 - Ship only the crc table from bz2 and use it in the updater
59 bytes, text/x-review-board-request
glandium
: review+
robert.strong.bugs
: review+
Details
Bug 1418425 - Remove libbz2 files as we were only using it for crc32 as the updater moved to lzma
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1418425 - Compile zlib as host lib
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1418425 - Do not build bsdiff on Android. The updater doesn't use it (we ship apk directly)
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1418425 - Update bsdiff to use the crc table directly and not link against bz2
59 bytes, text/x-review-board-request
glandium
: review+
Details
https://marco-c.github.io/code-coverage-reports/#modules/libbz2/src shows that most of the files are not used. Removing them and building it seems to be ok!
I took the option to remove the files. I could just remove them from moz.build but this would be misleading.
I suspect they're mostly used in the mar tools and the updater. Are those covered by the coverage reports?
Is all the updater code in toolkit/mozapps/update? We do have coverage for those files.
The updater doesn't use bz2 compression as of bug 641212. The remaining uses of the bz2 codebase are for BZ2_crc32Table (in the updater and in bsdiff). I'm pretty sure we could even replace the crc32 implementation in both to not use the symbol, entirely remove bz2 and the --enable-system-bz2 option.
Heck, there's even a crc32 function in zlib that we could use instead.
Even better! I will do that!
Summary: Most of the bz2 files are unused or don't have tests → Remove bz2 and use the zlib crc32 function instead
Comment on attachment 8929542 [details] Bug 1418425 - Update bsdiff to use libz instead of bz2 for the crc algo Per recent comments, it looks like new patches are incoming. So I'm not even going to look at this.
Attachment #8929542 - Flags: review?(gps)
Attachment #8929542 - Flags: review?(gps)
Comment on attachment 8929542 [details] Bug 1418425 - Update bsdiff to use libz instead of bz2 for the crc algo https://reviewboard.mozilla.org/r/200818/#review208246 ::: other-licenses/bsdiff/bsdiff.c:40 (Diff revision 2) > +// Do not use the by-four optim > +#define TBLS 1 > +#include "zutil.h" > +#include "crc32.h" zlib.h exports a crc32 function, just use that (just be aware that it works slightly differently, see the documentation in zlib.h). ::: other-licenses/bsdiff/moz.build:20 (Diff revision 2) > 'ws2_32', > ] > USE_STATIC_LIBS = True > > LOCAL_INCLUDES += [ > + '/modules/zlib/src', Use MOZ_ZLIB_CFLAGS
Attachment #8929542 - Flags: review-
Comment on attachment 8931717 [details] Bug 1418425 - Ship only the crc table from bz2 and use it in the updater https://reviewboard.mozilla.org/r/202854/#review208248 Same comments as previous patch.
Attachment #8931717 - Flags: review-
Comment on attachment 8929542 [details] Bug 1418425 - Update bsdiff to use libz instead of bz2 for the crc algo https://reviewboard.mozilla.org/r/200818/#review208316 ::: other-licenses/bsdiff/bsdiff.c:43 (Diff revision 3) > #define MIN(x,y) (((x)<(y)) ? (x) : (y)) > > -/*---------------------------------------------------------------------------*/ > - > -/* This variable lives in libbz2. It's declared in bzlib_private.h, so we just > - * declare it here to avoid including that entire header file. > +#define TBLS 1 > +#include "zutil.h" > +#include "crc32.h" > +#include "zlib.h" you shouldn't need more than zlib.h, an you shouldn't need to define TBLS. ::: other-licenses/bsdiff/bsdiff.c:226 (Diff revision 3) > 0, 0, 0, 0, 0, 0 > }; > > uint32_t numtriples; > > + unsigned int crc = crc32(0L, Z_NULL, 0); you want to set scrc here, or change the second call to use the result of this.
Attachment #8929542 - Flags: review-
Comment on attachment 8929542 [details] Bug 1418425 - Update bsdiff to use libz instead of bz2 for the crc algo https://reviewboard.mozilla.org/r/200818/#review208316 > you shouldn't need more than zlib.h, an you shouldn't need to define TBLS. Yeah, this commit was for testing the builds on other platforms. Wasn't ready for review :)
Attachment #8931986 - Flags: review?(mh+mozilla)
Attachment #8931718 - Flags: review?(mh+mozilla)
Attachment #8934543 - Flags: review?(mh+mozilla)
Comment on attachment 8931986 [details] Bug 1418425 - Compile zlib as host lib https://reviewboard.mozilla.org/r/203044/#review211246
Attachment #8931986 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8929542 [details] Bug 1418425 - Update bsdiff to use libz instead of bz2 for the crc algo https://reviewboard.mozilla.org/r/200818/#review211248 ::: other-licenses/bsdiff/moz.build:22 (Diff revision 4) > USE_STATIC_LIBS = True > > LOCAL_INCLUDES += [ > '/toolkit/mozapps/update/updater', > ] > +if not CONFIG['MOZ_ZLIB_CFLAGS']: you might as well skip the not and invert the if/else branches. ::: other-licenses/bsdiff/moz.build:27 (Diff revision 4) > +if not CONFIG['MOZ_ZLIB_CFLAGS']: > + LOCAL_INCLUDES += ['/modules/zlib/src'] > +else: > + HOST_CFLAGS += CONFIG['MOZ_ZLIB_CFLAGS'] > > -HOST_CXXFLAGS += CONFIG['MOZ_BZ2_CFLAGS'] > +HOST_USE_LIBS += [ 'hostzlib' ] --with-system-zlib builds won't have that hostzlib library, and will fail as a consequence.
Attachment #8929542 - Flags: review-
Comment on attachment 8931717 [details] Bug 1418425 - Ship only the crc table from bz2 and use it in the updater https://reviewboard.mozilla.org/r/202854/#review211250 Note that I'm likely to kill update-common.mozbuild soon. ::: toolkit/mozapps/update/updater/updater-common.build:107 (Diff revision 5) > LOCAL_INCLUDES += [ > '/toolkit/mozapps/update/common', > '/xpcom/base', # for nsVersionComparator.cpp > ] > > +if not CONFIG['MOZ_ZLIB_CFLAGS']: Same comment as previous patch, you can invert the if/else branches. ::: toolkit/mozapps/update/updater/updater-common.build:110 (Diff revision 5) > ] > > +if not CONFIG['MOZ_ZLIB_CFLAGS']: > + LOCAL_INCLUDES += ['/modules/zlib/src'] > +else: > + HOST_CXXFLAGS += CONFIG['MOZ_ZLIB_CFLAGS'] There's not host source built here. You want CXXFLAGS.
Attachment #8931717 - Flags: review-
Comment on attachment 8931718 [details] Bug 1418425 - Remove libbz2 files as we were only using it for crc32 as the updater moved to lzma https://reviewboard.mozilla.org/r/202856/#review211254
Attachment #8931718 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8934543 [details] Bug 1418425 - Do not build bsdiff on Android. The updater doesn't use it (we ship apk directly) https://reviewboard.mozilla.org/r/205448/#review211256
Attachment #8934543 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #34) > Comment on attachment 8931717 [details] > Bug 1418425 - Update the updater to use libz instead of bz2 for the crc algo > > https://reviewboard.mozilla.org/r/202854/#review211250 > > Note that I'm likely to kill update-common.mozbuild soon. Ah no, that's another updater-related file that I'm going to kill, you can ignore this comment.
Comment on attachment 8929542 [details] Bug 1418425 - Update bsdiff to use libz instead of bz2 for the crc algo https://reviewboard.mozilla.org/r/200818/#review212038 ::: other-licenses/bsdiff/moz.build:23 (Diff revision 5) > > LOCAL_INCLUDES += [ > '/toolkit/mozapps/update/updater', > ] > +if CONFIG['MOZ_ZLIB_CFLAGS']: > + HOST_CFLAGS += CONFIG['MOZ_ZLIB_CFLAGS'] without some ldflags this is going to fail --with-system-zlib. Please do a linux try build with --with-system-zlib set (although try without patches first, to check those builds actually work on automation).
Attachment #8929542 - Flags: review?(mh+mozilla) → review-
Comment on attachment 8931717 [details] Bug 1418425 - Ship only the crc table from bz2 and use it in the updater https://reviewboard.mozilla.org/r/202854/#review212042
Attachment #8931717 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8929542 [details] Bug 1418425 - Update bsdiff to use libz instead of bz2 for the crc algo https://reviewboard.mozilla.org/r/200818/#review212948
Attachment #8929542 - Flags: review?(mh+mozilla) → review+
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/656649ecfb11 Compile zlib as host lib r=glandium https://hg.mozilla.org/integration/autoland/rev/fd4af65bf966 Update bsdiff to use libz instead of bz2 for the crc algo r=glandium https://hg.mozilla.org/integration/autoland/rev/1d798e14223b Update the updater to use libz instead of bz2 for the crc algo r=glandium https://hg.mozilla.org/integration/autoland/rev/f29f7bb3059a Remove libbz2 files as we were only using it for crc32 as the updater moved to lzma r=glandium https://hg.mozilla.org/integration/autoland/rev/228743018ded Do not build bsdiff on Android. The updater doesn't use it (we ship apk directly) r=glandium
Backed out 5 changesets (bug 1418425) for Windows updater bustages and xpshell failures on toolkit/mozapps/update/tests/unit_base_updater/marSuccessPartial.js r=backout on a CLOSED TREE Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=228743018ded362f84bd71fa97f688068422abab Recent failure log: https://treeherder.mozilla.org/logviewer.html#/?repo=autoland&job_id=151286727&lineNumber=8052 Backout: https://hg.mozilla.org/integration/autoland/rev/c17fd6ccda7eec43bad45ec1f4fbb7ad7f1ee05f
Flags: needinfo?(sledru)
So, it seems that the crc32 algo that we use as to be the same as the releng side. Therefor, for the updater, I copied the crctable from bz2 into toolkit/mozapps/update/updater/crctable.h, added the license file and kept the crc32 function which was used in updater.cpp 845 See: https://reviewboard.mozilla.org/r/202854/ Both tests work for me now with the change: ./mach test toolkit/mozapps/update/tests/unit_base_updater/marSuccessPartial.js ./mach test toolkit/mozapps/update/tests/unit_base_updater/marFailurePartial.js
Flags: needinfo?(sledru)
Comment on attachment 8931717 [details] Bug 1418425 - Ship only the crc table from bz2 and use it in the updater https://reviewboard.mozilla.org/r/202854/#review217008 ::: toolkit/mozapps/update/updater/BZ2_LICENSE:4 (Diff revision 10) > +This program, "bzip2", the associated library "libbzip2", and all > +documentation, are copyright (C) 1996-2010 Julian R Seward. All > +rights reserved. This paragraph is kind of misleading. OTOH, this file is only there because of the crctable header, but this license actually barely applies to that file. Why? Because crctable.h contains a lookup table for a polynomial, and ways to generate that table have been public since before bzip2 was published. For example, there's this document from 1993: http://www.ross.net/crc/download/crc_v3.txt It contains a public domain generator for the crc-32 table, and if you take crcmodel.[ch] and crctable.c from there, and change TB_REVER to FALSE, you get the exact same table as the one in the bzip2 source. IOW, the table is essentially public domain, and you don't need all those license boilerplates. (As for why bzip2 and gzip crc-32 are not compatible, it's because despite using the same crc, they are actually using reversed polynomials, which don't generate the same results)
Comment on attachment 8931717 [details] Bug 1418425 - Ship only the crc table from bz2 and use it in the updater https://reviewboard.mozilla.org/r/202854/#review218070
Attachment #8931717 - Flags: review?(mh+mozilla) → review+
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/218077e82115 Compile zlib as host lib r=glandium https://hg.mozilla.org/integration/autoland/rev/29fd023f999f Update bsdiff to use libz instead of bz2 for the crc algo r=glandium https://hg.mozilla.org/integration/autoland/rev/772538a846cb Ship only the crc table from bz2 and use it in the updater r=glandium https://hg.mozilla.org/integration/autoland/rev/d32ae48690a7 Remove libbz2 files as we were only using it for crc32 as the updater moved to lzma r=glandium https://hg.mozilla.org/integration/autoland/rev/8154486d3e00 Do not build bsdiff on Android. The updater doesn't use it (we ship apk directly) r=glandium
Comment on attachment 8929542 [details] Bug 1418425 - Update bsdiff to use libz instead of bz2 for the crc algo https://reviewboard.mozilla.org/r/200818/#review218236 ::: other-licenses/bsdiff/moz.build:16 (Diff revision 9) > - ] > - > if CONFIG['HOST_OS_ARCH'] == 'WINNT': > HOST_OS_LIBS += [ > 'ws2_32', > + 'vcruntime', I added that to fix the build issue. Try confirmed that it fixed the issue.
Flags: needinfo?(sledru)
Attachment #8931717 - Flags: review?(robert.strong.bugs)
Comment on attachment 8929542 [details] Bug 1418425 - Update bsdiff to use libz instead of bz2 for the crc algo https://reviewboard.mozilla.org/r/200818/#review218252
Attachment #8929542 - Flags: review+
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/142001babb39 Compile zlib as host lib r=glandium https://hg.mozilla.org/integration/autoland/rev/47fbcaf5ad05 Update bsdiff to use libz instead of bz2 for the crc algo r=andi,glandium https://hg.mozilla.org/integration/autoland/rev/2e95ebdcae6d Ship only the crc table from bz2 and use it in the updater r=glandium https://hg.mozilla.org/integration/autoland/rev/7b7725d1d3c3 Remove libbz2 files as we were only using it for crc32 as the updater moved to lzma r=glandium https://hg.mozilla.org/integration/autoland/rev/0ecc98bd0822 Do not build bsdiff on Android. The updater doesn't use it (we ship apk directly) r=glandium
See Also: → 1430535
Sebastian, could you please backout this patches from m-c & mb? Bug 1430535 is a blocker and we will need time to figure out the best solution here. (and also tests covering that stuff in m-c)
Flags: needinfo?(aryx.bugmail)
It should automatically get synced to beta.
Flags: needinfo?(aryx.bugmail)
Target Milestone: mozilla59 → ---
Backout by ncsoregi@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/a00e3a3dbad4 Backed out 5 changesets requested per comment #83. on a CLOSED TREE a=backout
This blackout has landed on m-c, and is in tonight's Nightly build: cset https://hg.mozilla.org/mozilla-central/rev/8e33bdf820dcc2ecd6f326eb83c39fc5e4769dcf Nightly is not finding this latest build - keeps telling me its up-to-date. Will a manual download/update be required to get back on track ?
Jim, see bug 1430535 about the issue itself.
Mike, I have a few ideas to fix the issue: 1) As Simon suggested, we could use the two crc32 functions: 1) check with the bz2 impl 2) check with the zlib impl 3) fails the check The next time we have a watershed, we moved to that. 2) Keep the bz2 implementation in updater & bsdiff (for now, it is just in the updater) We could create a new directory with the crctable and the algo. 3) keep everything as it and just delete unused bz2 file. (very similar to the previous one) 4) Migrate all to zlib but this requires a watershed and complex mechanics for a small gain Mike, what do you think?
Flags: needinfo?(mh+mozilla)
It would be a good thing to land this on oak and verify that everything is working by updating clients prior to landing on nightly.
I wouldn't be surprised if the crc generated with larger file sizes is broken in the copied implementation since the ci tests do check the crc on small files and they passed. Ideally, the reason why the copied implementation failed would be figured out instead of implementing comment #89.
Is the CI using bsdiff to generate test files?
(In reply to Sylvestre Ledru [:sylvestre] from comment #89) > 2) Keep the bz2 implementation in updater & bsdiff (for now, it is just in > the updater) This is the best option IMHO. > We could create a new directory with the crctable and the algo. Note there's CRC32C (yet different polynomials than zlib and bz2) in xpcom/io/.
Flags: needinfo?(mh+mozilla)
(In reply to Masatoshi Kimura [:emk] from comment #92) > Is the CI using bsdiff to generate test files? Yes
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #94) > (In reply to Masatoshi Kimura [:emk] from comment #92) > > Is the CI using bsdiff to generate test files? > Yes Really? The CI didn't fail even if I changed crc32() in bsdiff.c to a dummy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a79e9f10296de16a7b5fee28bca589407549caef
(In reply to Masatoshi Kimura [:emk] from comment #95) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #94) > > (In reply to Masatoshi Kimura [:emk] from comment #92) > > > Is the CI using bsdiff to generate test files? > > Yes > > Really? The CI didn't fail even if I changed crc32() in bsdiff.c to a dummy: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=a79e9f10296de16a7b5fee28bca589407549caef The mbsdiff host binary used by the build system is not tested but the test mar files are generated using the mbsdiff binary. Also see comment #91
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #96) > (In reply to Masatoshi Kimura [:emk] from comment #95) > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > > comment #94) > > > (In reply to Masatoshi Kimura [:emk] from comment #92) > > > > Is the CI using bsdiff to generate test files? > > > Yes > > > > Really? The CI didn't fail even if I changed crc32() in bsdiff.c to a dummy: > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=a79e9f10296de16a7b5fee28bca589407549caef > The mbsdiff host binary used by the build system is not tested but the test > mar files are generated using the mbsdiff binary. Also see comment #91 The bzip2 crc32 is simply incompatible with the zlib crc32 (see comment #67). <https://hg.mozilla.org/integration/autoland/rev/47fbcaf5ad05> changed the bzip2 crc32 to the zlib crc32 in bsdiff.c, so it did no longer match the updator's crc32 that is still using bzip2 crc32. But the CI did not catch it because the CI does not test the mbsdiff binary. So we have to implement comment #89.
Is there no way to use a crc32 that is compatible with the existing crc32 instead of using the zlib crc32 which is incompatible?
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #98) > Is there no way to use a crc32 that is compatible with the existing crc32 > instead of using the zlib crc32 which is incompatible? Isn't it 2) or 3) of comment #89?
Essentially 2 since that would make it so a watershed is not necessary. I pushed the existing patches as one push to oak with minor changes that I think should do it. Should be able to check if all is well tomorrow. https://hg.mozilla.org/projects/oak/rev/48af9e5146eeb412d8a367cceac3413ca9e672fe https://treeherder.mozilla.org/#/jobs?repo=oak
This is the patch I had locally. The only question was about crctable.h, should it remain updater or be moved into a specific directory.
I prefer keeping shared files like that with the client code so leave it. Please also verify that updates work on oak tomorrow, etc.
Sure, will do that! Thanks
One of the concerns here is that the msbdiff binary from mozilla-central got used on -release and -beta (and maybe even on esr, unsure there no build started during this problem) So please coordinate with releng if we modify the crc used by the msbdiff on central, even if the Gecko 60 updater is modified to support both formats. We'll figure out the solution on our automations end, but will need to be in the loop here.
Just remembered that oak isn't set up to create partial mar files so I compared the patch files created by the mbsdiff with and without the changes and they have the same output. So, this should be good to go.
Also, about:license should be updated to remove "The bzip2 compression library (Julian Seward)" but please do that in another bug.
Product: Core → Firefox Build System
Sylvestre, the changes from the original patch make it so the crc that is created by mbsdiff is the same as the one created by the version of mbsdiff currently in use by the build system. With that in mind is there any reason not to get this landed?
Flags: needinfo?(sledru)
I have been distracted by other things. I will try to work on that this week or next week.
Flags: needinfo?(sledru)
Try seem happy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=407c4e6c60ed6697eb5d01573df183fbf72caea9&selectedJob=175129969 (at least for the build) I will try to land it once nightly is 62
So, (In reply to Sylvestre Ledru [:sylvestre] from comment #110) > Comment on attachment 8931986 [details] > Bug 1418425 - Compile zlib as host lib > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/203044/diff/7-8/ This patch should no longer be needed.
Could you update the patches so instead of reverting changes the changes are not made in the first place?
Flags: needinfo?(sledru)
yeah, i was planning to do that before landing. Here, it was to clearly show the difference between what I had on mozreview and what landed on oak!
Flags: needinfo?(sledru)
Comment on attachment 8970168 [details] Bug 1418425 - Update bsdiff to use the crc table directly and not link against bz2 https://reviewboard.mozilla.org/r/238968/#review245450
Attachment #8970168 - Flags: review+
Comment on attachment 8931717 [details] Bug 1418425 - Ship only the crc table from bz2 and use it in the updater https://reviewboard.mozilla.org/r/202854/#review245454 r=me with the changes that aren't needed or are reverted in later patches aren't landed.
Attachment #8931717 - Flags: review?(robert.strong.bugs) → review+
Attachment #8929542 - Attachment is obsolete: true
Comment on attachment 8970168 [details] Bug 1418425 - Update bsdiff to use the crc table directly and not link against bz2 https://reviewboard.mozilla.org/r/238968/#review248096 ::: other-licenses/bsdiff/bsdiff.c:35 (Diff revision 2) > #include <unistd.h> > #include <arpa/inet.h> > #define _O_BINARY 0 > #endif > > +#include "crctable.h" This file is added in part 3, but this is part 2. Also, the commit message is outdated.
Attachment #8970168 - Flags: review?(mh+mozilla)
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c5acba2f11e0 Compile zlib as host lib r=glandium https://hg.mozilla.org/integration/autoland/rev/932ea532bb41 Ship only the crc table from bz2 and use it in the updater r=glandium,rstrong https://hg.mozilla.org/integration/autoland/rev/40d6bf73191f Update bsdiff to use the crc table directly and not link against bz2 r=rstrong https://hg.mozilla.org/integration/autoland/rev/652c4066dab8 Remove libbz2 files as we were only using it for crc32 as the updater moved to lzma r=glandium https://hg.mozilla.org/integration/autoland/rev/d361bb2907bc Do not build bsdiff on Android. The updater doesn't use it (we ship apk directly) r=glandium
Attachment #8970168 - Flags: review?(mh+mozilla) → review+
Blocks: 1459851
Landed a week ago, looks like we are good now!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: