----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45806/#review127432 -----------------------------------------------------------
Thanks for working on this! I've left some comments below. In addition to these comments, could you pelase update the summary and description to better reflect what this change is doing now. It's not really duplicating binaries, but rather renaming the exisiting files from which these bianreis are generated from `s/slave/agent` and updating `configure.ac` and `src/Makefile.am` to generate duplicate copies of the binaries (one with the slave naming, and one with the agent naming). configure.ac (line 81) <https://reviews.apache.org/r/45806/#comment190732> I would probably move this line up to keep this grouping alphabetical. configure.ac (lines 82 - 84) <https://reviews.apache.org/r/45806/#comment190728> You should probably mark this with an explicit TODO. You should probably also refer to it as a script rather than a 'shell'. Also, all of these lines that wrap (including the existing ones) should only be using two space indents instead of a tab. if you could fix that throughout, that would be great. ``` # TODO(dongdong): Remove this script once the term # 'slave' is deprecated (MESOS-3782). AC_CONFIG_FILES([bin/gdb-mesos-slave.sh:bin/gdb-mesos-agent.sh.in], [chmod +x bin/gdb-mesos-slave.sh]) ``` configure.ac (line 88) <https://reviews.apache.org/r/45806/#comment190733> Same comment about keeping the group alphabetical as above. configure.ac (lines 89 - 90) <https://reviews.apache.org/r/45806/#comment190725> Same comment about the TODO as above. configure.ac (line 94) <https://reviews.apache.org/r/45806/#comment190735> Same comment about keeping the group alphabetical as above. configure.ac (lines 95 - 97) <https://reviews.apache.org/r/45806/#comment190726> Same comment about the TODO and the wrapping as above. configure.ac (line 102) <https://reviews.apache.org/r/45806/#comment190736> Same comment about keeping the group alphabetical as above. configure.ac (lines 103 - 105) <https://reviews.apache.org/r/45806/#comment190729> Same comment about the TODO and the wrapping as above. configure.ac (line 109) <https://reviews.apache.org/r/45806/#comment190737> Same comment about keeping the group alphabetical as above. Also, the wrapping. configure.ac (lines 111 - 112) <https://reviews.apache.org/r/45806/#comment190740> Same comment about the TODO as above. configure.ac (line 121) <https://reviews.apache.org/r/45806/#comment190738> Same comment about keeping the group alphabetical as above. configure.ac (lines 122 - 124) <https://reviews.apache.org/r/45806/#comment190741> Same comment about the TODO and the wrapping as above. configure.ac (line 127) <https://reviews.apache.org/r/45806/#comment190739> Same comment about keeping the group alphabetical as above. configure.ac (lines 128 - 130) <https://reviews.apache.org/r/45806/#comment190742> Same comment about the TODO and the wrapping as above. src/Makefile.am (line 1146) <https://reviews.apache.org/r/45806/#comment190743> Please wrap this in a TODO as the comments above suggest. src/Makefile.am (lines 1152 - 1156) <https://reviews.apache.org/r/45806/#comment190744> I would probably move this up to keep things alphabetical. src/Makefile.am (line 1301) <https://reviews.apache.org/r/45806/#comment190745> I would probably move this up to keep things alphabetical in the group. src/Makefile.am (line 1305) <https://reviews.apache.org/r/45806/#comment190746> Same comment as above about keeping things alphabetical. src/Makefile.am (line 1311) <https://reviews.apache.org/r/45806/#comment190748> Same comment as above about keeping things alphabetical. src/Makefile.am (line 1318) <https://reviews.apache.org/r/45806/#comment190749> Same comment as above about keeping things alphabetical. src/Makefile.am (lines 2069 - 2081) <https://reviews.apache.org/r/45806/#comment190773> I probably wouldn't overload this make target to copy in the template. Instead, I'd create a new target to do the copying and then make the `install-data-hook` target depend on it, i.e.: ``` # Copy mesos-agent-env.sh.template to mesos-slave-env.sh.template. # # TODO(dongdong): Remove this target once the # slave->agent rename is complete(MESOS-3782). copy-agent-env-template: cp $(pkgsysconfdir)/mesos-agent-env.sh.template \ $(pkgsysconfdir)/mesos-slave-env.sh.template # Install compatibility symlinks for modules that used to be in $(LIBDIR) # but are now in $(PKGMODULEDIR). We use install-data-hook because it # runs late in the install process after the target directories have # been created. install-data-hook: copy-agent-env-template ... ``` I was originally thinking this might be one of the cases where we maybe want to just copy the file as-is instead of relying on configure/make magic to "generate" the slave variant of the file from an agent input file. However, it's probably better to do things this way because it forces renames of **all** files in the code base to `s/slave/agent` and isolates all future changes after the deprecation cycle to removing lines from `configure.ac` and `src/Makefile.am`. The fewer places we need to make changes in the future, the better. src/Makefile.am (line 2079) <https://reviews.apache.org/r/45806/#comment190763> I would probably align the "\" with the rest of them. Set your tab-space to 8 to see them line up. - Kevin Klues On April 6, 2016, 8:33 a.m., zhou xing wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45806/ > ----------------------------------------------------------- > > (Updated April 6, 2016, 8:33 a.m.) > > > Review request for mesos, Kevin Klues and Vinod Kone. > > > Bugs: MESOS-3782 > https://issues.apache.org/jira/browse/MESOS-3782 > > > Repository: mesos > > > Description > ------- > > [#mesos-3782] Duplicate the following binaries: > 1. executable files > 2. shell scripts under mesos/bin folder > 3. shell scripts under mesos/src/deploy folder > > > Diffs > ----- > > bin/gdb-mesos-slave.sh.in dbeec8464b26bd808f7a50b8412a2778f1458f22 > bin/lldb-mesos-slave.sh.in 896c411b2b05d3c4a14288002520a5391a88d955 > bin/mesos-local-flags.sh.in ab5b6c8bd8847485c5a47d637c9f4fe88c59ae65 > bin/mesos-slave-flags.sh.in > bin/mesos-slave.sh.in 1e3b748ed4dc32ba6bd8adece20f439bce38abc3 > bin/valgrind-mesos-slave.sh.in 900c5883d96cf14e121e566bcf1ad4dc9a47abf6 > configure.ac c693b825294f82ace8a14563cf2229820e159e3c > src/Makefile.am 55d3b341361bed25f3aa966d77060c88be29e5b0 > src/deploy/mesos-deploy-env.sh.template > bea5584fbcc68825b1c35b370aed17b0e432edd5 > src/deploy/mesos-slave-env.sh.template > 31567d6a47e19385aed56edfc7e67457c8cdde3e > src/deploy/mesos-start-cluster.sh.in > f7a003d9a8e5bbb4f11353988e55e715da0b2b4f > src/deploy/mesos-start-slaves.sh.in > 50860f40e33fcbb1787be6c035873de4bcc83de5 > src/deploy/mesos-stop-cluster.sh.in > e5f8c1fb400c56715774889632aa74d9eac33645 > src/deploy/mesos-stop-slaves.sh.in 3dd9b51edff2beb3ccc8d5dd44f0cdc265f623f9 > > Diff: https://reviews.apache.org/r/45806/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > zhou xing > >
