Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2922273002:
Introduce Zucchini in chromium (Closed)
DescriptionIntroduce Zucchini in chromium
Zucchini is a patching tool inspired by Courgette, for delta compression of archives containing common exexecutable formats.
This is the first step of integrating Zucchini in Chrome Installer.
BUG=729154
Review-Url: https://codereview.chromium.org/2922273002
Cr-Commit-Position: refs/heads/master@{#480997}
Committed: https://chromium.googlesource.com/chromium/src/+/aef136ba6ee33e2964d30232785bb97d96d50162
Patch Set 1 #Patch Set 2 : Change owner etiennep to etiennep@chromium.org #Patch Set 3 : Move zucchini into chrome/installer #Patch Set 4 : Reorder OWNERS and remove erikchen@ #
Total comments: 4
Patch Set 5 : add process_version_rc_template #Patch Set 6 : Rename zucchini_main #Patch Set 7 : Add zucchini_exe_version dependency in zucchini #
Total comments: 16
Patch Set 8 : CR-grt #Patch Set 9 : Move zucchini deps in top level BUILD #Patch Set 10 : cl format #
Total comments: 3
Patch Set 11 : add Component in OWNERS file #
Total comments: 1
Messages
Total messages: 37 (18 generated)
Description was changed from ========== Introduce Zucchini in chromium Zucchini is a patching tool inspired by Courgette, for delta compression of archives containing common exexecutable formats. This is the first step of integrating Zucchini in Chrome Installer. BUG=729154 ========== to ========== Introduce Zucchini in chromium Zucchini is a patching tool inspired by Courgette, for delta compression of archives containing common exexecutable formats. This is the first step of integrating Zucchini in Chrome Installer. BUG=729154 ==========
etiennep@google.com changed reviewers: + huangs@chromium.org
etiennep@google.com changed reviewers: + grt@chromium.org
The CQ bit was checked by etiennep@google.com to run a CQ dry run
The CQ bit was unchecked by etiennep@google.com
https://codereview.chromium.org/2922273002/diff/60001/chrome/installer/zucchi... File chrome/installer/zucchini/BUILD.gn (right): https://codereview.chromium.org/2922273002/diff/60001/chrome/installer/zucchi... chrome/installer/zucchini/BUILD.gn:9: "main.cc", nit: zucchini_main.cc https://codereview.chromium.org/2922273002/diff/60001/chrome/installer/zucchi... chrome/installer/zucchini/BUILD.gn:15: } please add a process_version_rc_template target and the right dependencies (see setup/BUILD.gn for an example) so that this binary has a version resource. you'll probably need if_win sections for it. i'm not sure if other platforms have their own ways of doing similar things.
On 2017/06/08 20:15:53, grt (UTC plus 2) wrote: > https://codereview.chromium.org/2922273002/diff/60001/chrome/installer/zucchi... > File chrome/installer/zucchini/BUILD.gn (right): > > https://codereview.chromium.org/2922273002/diff/60001/chrome/installer/zucchi... > chrome/installer/zucchini/BUILD.gn:9: "main.cc", > nit: zucchini_main.cc > > https://codereview.chromium.org/2922273002/diff/60001/chrome/installer/zucchi... > chrome/installer/zucchini/BUILD.gn:15: } > please add a process_version_rc_template target and the right dependencies (see > setup/BUILD.gn for an example) so that this binary has a version resource. > you'll probably need if_win sections for it. i'm not sure if other platforms > have their own ways of doing similar things. PTAL
I failed to publish my drafts... Here they are. https://codereview.chromium.org/2922273002/diff/60001/chrome/installer/zucchi... File chrome/installer/zucchini/BUILD.gn (right): https://codereview.chromium.org/2922273002/diff/60001/chrome/installer/zucchi... chrome/installer/zucchini/BUILD.gn:9: "main.cc", On 2017/06/08 20:15:53, grt (UTC plus 2) wrote: > nit: zucchini_main.cc Done. https://codereview.chromium.org/2922273002/diff/60001/chrome/installer/zucchi... chrome/installer/zucchini/BUILD.gn:15: } On 2017/06/08 20:15:53, grt (UTC plus 2) wrote: > please add a process_version_rc_template target and the right dependencies (see > setup/BUILD.gn for an example) so that this binary has a version resource. > you'll probably need if_win sections for it. i'm not sure if other platforms > have their own ways of doing similar things. Done.
https://codereview.chromium.org/2922273002/diff/120001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2922273002/diff/120001/BUILD.gn#newcode429 BUILD.gn:429: "//chrome/installer/zucchini:zucchini", is the intent to have this built for win, mac, and linux desktop (not chromeos)? if so, i think using an (is_win || is_mac || (is_linux && !is_chromeos && !is_chromecast)) block on its own is slightly more clear. if i'm reading the tea leaves properly, this will exclude ios, android, fuchsia, chromecast, and nacl. please also add a comment around the conditional so that folks know how to adjust it if a new platform comes along. there may be other things that really belong in a block like that. https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... File chrome/installer/zucchini/BUILD.gn (right): https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/BUILD.gn:10: "zucchini_main.cc", i see that //chrome/installer/setup:setup lists the .rc.version file as a source. presumably this is so that the "setup" target is rebuilt if the .rc.version file changes. this seems like the sort of thing that process_version_rc_template *should* take care of, but it's not obvious to me that it does. would you try building zucchini, touching the .rc.version file, then building zucchini again (or use "ninja -v -d explain" or something like that) to make sure that it's rebuilt in that case? if it isn't, please add the .rc.version file as an explicit source here. https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/BUILD.gn:14: "//base", this doesn't yet depend on base. please remove this until it does. https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/BUILD.gn:36: "//testing/gtest", similar comment -- this doesn't yet directly depend on gtest. please add the dependency when it is needed. https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... File chrome/installer/zucchini/zucchini_exe_version.rc.version (right): https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/zucchini_exe_version.rc.version:30: VALUE "FileDescription", "@PRODUCT_INSTALLER_FULLNAME@" PRODUCT_INSTALLER_FULLNAME = "Chrome Installer". I think it's safe to make this "Zucchini" since the name is brand-independent. https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/zucchini_exe_version.rc.version:32: VALUE "InternalName", "setup" "zucchini" https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/zucchini_exe_version.rc.version:34: VALUE "ProductName", "@PRODUCT_INSTALLER_FULLNAME@" "Zucchini" https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/zucchini_exe_version.rc.version:37: VALUE "ProductShortName", "@PRODUCT_INSTALLER_SHORTNAME@" "Zucchini"
I applied some correction. I will put this CL on hold until we reach out to chrome-design-docs@ and decide for good where Zucchini should go! https://codereview.chromium.org/2922273002/diff/120001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2922273002/diff/120001/BUILD.gn#newcode429 BUILD.gn:429: "//chrome/installer/zucchini:zucchini", On 2017/06/09 08:22:53, grt (UTC plus 2) wrote: > is the intent to have this built for win, mac, and linux desktop (not chromeos)? > if so, i think using an (is_win || is_mac || (is_linux && !is_chromeos && > !is_chromecast)) block on its own is slightly more clear. if i'm reading the tea > leaves properly, this will exclude ios, android, fuchsia, chromecast, and nacl. > please also add a comment around the conditional so that folks know how to > adjust it if a new platform comes along. there may be other things that really > belong in a block like that. I'd rather avoid creating a new conditional block. Maybe the already existing (!is_ios && !is_android && !is_chromecast && !is_fuchsia) makes sense. I will investigate more on this! https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... File chrome/installer/zucchini/BUILD.gn (right): https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/BUILD.gn:10: "zucchini_main.cc", On 2017/06/09 08:22:53, grt (UTC plus 2) wrote: > i see that //chrome/installer/setup:setup lists the .rc.version file as a > source. presumably this is so that the "setup" target is rebuilt if the > .rc.version file changes. this seems like the sort of thing that > process_version_rc_template *should* take care of, but it's not obvious to me > that it does. would you try building zucchini, touching the .rc.version file, > then building zucchini again (or use "ninja -v -d explain" or something like > that) to make sure that it's rebuilt in that case? if it isn't, please add the > .rc.version file as an explicit source here. Touching .rc.version effectively causes ninja to rebuild the executable, and updates its informations - I believe that is because zucchini_exe_version appears in deps. https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/BUILD.gn:14: "//base", On 2017/06/09 08:22:53, grt (UTC plus 2) wrote: > this doesn't yet depend on base. please remove this until it does. Done. https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/BUILD.gn:36: "//testing/gtest", On 2017/06/09 08:22:53, grt (UTC plus 2) wrote: > similar comment -- this doesn't yet directly depend on gtest. please add the > dependency when it is needed. Done. https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... File chrome/installer/zucchini/zucchini_exe_version.rc.version (right): https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/zucchini_exe_version.rc.version:30: VALUE "FileDescription", "@PRODUCT_INSTALLER_FULLNAME@" On 2017/06/09 08:22:53, grt (UTC plus 2) wrote: > PRODUCT_INSTALLER_FULLNAME = "Chrome Installer". I think it's safe to make this > "Zucchini" since the name is brand-independent. Done. https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/zucchini_exe_version.rc.version:32: VALUE "InternalName", "setup" On 2017/06/09 08:22:53, grt (UTC plus 2) wrote: > "zucchini" oups, hadn't seen those! https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/zucchini_exe_version.rc.version:34: VALUE "ProductName", "@PRODUCT_INSTALLER_FULLNAME@" On 2017/06/09 08:22:53, grt (UTC plus 2) wrote: > "Zucchini" Done. https://codereview.chromium.org/2922273002/diff/120001/chrome/installer/zucch... chrome/installer/zucchini/zucchini_exe_version.rc.version:37: VALUE "ProductShortName", "@PRODUCT_INSTALLER_SHORTNAME@" On 2017/06/09 08:22:53, grt (UTC plus 2) wrote: > "Zucchini" Done.
SGTM. Ping me when you're ready for the next step.
PTAnL
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
etiennep@google.com changed reviewers: + erikchen@chromium.org
etiennep@google.com changed reviewers: + scottmg@chromium.org
lgtm w/ a nit. please wait for overall discussion to settle before landing. thanks. https://codereview.chromium.org/2922273002/diff/180001/chrome/installer/zucch... File chrome/installer/zucchini/OWNERS (right): https://codereview.chromium.org/2922273002/diff/180001/chrome/installer/zucch... chrome/installer/zucchini/OWNERS:3: wfh@chromium.org could you add TEAM + COMPONENT (https://docs.google.com/document/d/1jty6UsFMW9-SYgpQC-ztEc3lltziOQBBArMkROCST...) to this as appropriate?
The CQ bit was checked by etiennep@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
scottmg@ can you take a look at BUILD.gn? https://codereview.chromium.org/2922273002/diff/180001/chrome/installer/zucch... File chrome/installer/zucchini/OWNERS (right): https://codereview.chromium.org/2922273002/diff/180001/chrome/installer/zucch... chrome/installer/zucchini/OWNERS:3: wfh@chromium.org On 2017/06/20 08:56:52, grt (UTC plus 2) wrote: > could you add TEAM + COMPONENT > (https://docs.google.com/document/d/1jty6UsFMW9-SYgpQC-ztEc3lltziOQBBArMkROCST...) > to this as appropriate? I added #COMPONENT. For team, I couldn't find a relevant @chromium.org and there aren't much examples elsewhere. Maybe you have a suggestion of a team I could put; or else, it seems like in most cases #TEAM is missing.
BUILD.gn lgtm
https://codereview.chromium.org/2922273002/diff/180001/chrome/installer/zucch... File chrome/installer/zucchini/OWNERS (right): https://codereview.chromium.org/2922273002/diff/180001/chrome/installer/zucch... chrome/installer/zucchini/OWNERS:3: wfh@chromium.org On 2017/06/20 20:54:04, etiennep wrote: > On 2017/06/20 08:56:52, grt (UTC plus 2) wrote: > > could you add TEAM + COMPONENT > > > (https://docs.google.com/document/d/1jty6UsFMW9-SYgpQC-ztEc3lltziOQBBArMkROCST...) > > to this as appropriate? > > I added #COMPONENT. For team, I couldn't find a relevant @chromium.org and there > aren't much examples elsewhere. Maybe you have a suggestion of a team I could > put; or else, it seems like in most cases #TEAM is missing. COMPONENT is good enough for now. thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by etiennep@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2922273002/#ps200001 (title: "add Component in OWNERS file")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1497997685522020,
"parent_rev": "cc6ae89f12dca8195dc58251a404de1411d89d46", "commit_rev":
"aef136ba6ee33e2964d30232785bb97d96d50162"}
Message was sent while issue was closed.
Description was changed from ========== Introduce Zucchini in chromium Zucchini is a patching tool inspired by Courgette, for delta compression of archives containing common exexecutable formats. This is the first step of integrating Zucchini in Chrome Installer. BUG=729154 ========== to ========== Introduce Zucchini in chromium Zucchini is a patching tool inspired by Courgette, for delta compression of archives containing common exexecutable formats. This is the first step of integrating Zucchini in Chrome Installer. BUG=729154 Review-Url: https://codereview.chromium.org/2922273002 Cr-Commit-Position: refs/heads/master@{#480997} Committed: https://chromium.googlesource.com/chromium/src/+/aef136ba6ee33e2964d30232785b... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/aef136ba6ee33e2964d30232785b...
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
Can we give this a non-clever name please?
Message was sent while issue was closed.
Certainly the name is not as clever as "Chrome". But what are you looking for?
Message was sent while issue was closed.
On 2017/06/28 14:22:23, huangs wrote: > Certainly the name is not as clever as "Chrome". But what are you looking for? A name that describes what this does, not a brand. Say binary_diff, binary_patch, or something like that. Brand names like "chrome", "blink" etc are ok if most people will know about them eventually. Using them for projects that likely only a handful of people will work on full-time makes life harder for everyone not working on them. (We have examples of those in the tree too, e.g. src/gin, but most things do have descriptive names.)
Message was sent while issue was closed.
I'd say this depends on the level of integration. For "installer", this makes sense since clearly it's "installer for Chrome". However, Zucchini is actually a stand-alone tool (with its own stand-alone executable -- although for patch application we'd include it as a library). Moreover also we plan to use the tool elsewhere. I do agree that "Zucchini" is a rather silly name, but it also pays homage to Courgette, from which we borrowed a lot of code in earlier development. For the longer term, I'd like to yank this out as a standalone third_party module. But for now, embedded into chrome/installer has the advantage of requiring more thorough code review (which you're more than welcome to partake in).
Message was sent while issue was closed.
On 2017/06/28 14:43:31, huangs wrote: > I'd say this depends on the level of integration. For "installer", this makes > sense since clearly it's "installer for Chrome". However, Zucchini is actually a > stand-alone tool (with its own stand-alone executable -- although for patch > application we'd include it as a library). Moreover also we plan to use the tool > elsewhere. I do agree that "Zucchini" is a rather silly name, but it also pays > homage to Courgette, from which we borrowed a lot of code in earlier > development. > > For the longer term, I'd like to yank this out as a standalone third_party > module. But for now, embedded into chrome/installer has the advantage of > requiring more thorough code review (which you're more than welcome to partake > in). I think this (moving code in chrome into their own repos) too is something that we used to do a lot early in chrome history and which is now more considered a mistake. We made many of our early things kind of general-purpose, independent projects (gyp, grit, etc), but in the end nobody except chrome really used them and this just made development harder and more fractured. If you do want to make this its own component, just ask for a git mirror of that subdirectory (and make sure you don't use base/ in that case – but you probably weren't planning on that anyways since it runs in the installer). That way we don't need any "change upstream, roll deps" dances for changes to it. (We had a thread for this somewhere, but I can't find it now. We did move e.g. grit into src/ due to the conclusion of that thread.)
Message was sent while issue was closed.
https://codereview.chromium.org/2922273002/diff/200001/chrome/installer/zucch... File chrome/installer/zucchini/OWNERS (right): https://codereview.chromium.org/2922273002/diff/200001/chrome/installer/zucch... chrome/installer/zucchini/OWNERS:2: etiennep@chromium.org etiennep: are you a chromium committer? if no, please remove yourself from this file as per the requirements in https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#.... i forgot to ask when i lgtm'd this CL. my apologies for the mistake. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
