You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alex Clemmer <cl...@gmail.com> on 2017/01/17 08:31:56 UTC
Review Request 55607: CMake: Added configuration of test scripts in
the bin/ directory.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55607/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Bugs: MESOS-6757
https://issues.apache.org/jira/browse/MESOS-6757
Repository: mesos
Description
-------
This resolves MESOS-6757.
Diffs
-----
cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc
Diff: https://reviews.apache.org/r/55607/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 55607: CMake: Added configuration of test scripts
in the bin/ directory.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55607/#review163364
-----------------------------------------------------------
Ship it!
Quickly verified:
```
cmake ..
ls bin # Looks correct
cat bin/mesos-agent.sh # Looks correct
```
cmake/MesosConfigure.cmake (lines 222 - 224)
<https://reviews.apache.org/r/55607/#comment234772>
We generally put `NOTE`s on a newline. And sometimes we put an extra newline before one (especially after big comment blocks).
- Joseph Wu
On Jan. 26, 2017, 1:20 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55607/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2017, 1:20 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Bugs: MESOS-6757
> https://issues.apache.org/jira/browse/MESOS-6757
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This resolves MESOS-6757.
>
>
> Diffs
> -----
>
> cmake/MesosConfigure.cmake ce127e1e8c5c33ad8badef6fb3ac9f50ade9056f
>
> Diff: https://reviews.apache.org/r/55607/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55607: CMake: Added configuration of test scripts
in the bin/ directory.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55607/
-----------------------------------------------------------
(Updated Jan. 26, 2017, 9:20 a.m.)
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Changes
-------
Address Joseph's questions.
Bugs: MESOS-6757
https://issues.apache.org/jira/browse/MESOS-6757
Repository: mesos
Description
-------
This resolves MESOS-6757.
Diffs (updated)
-----
cmake/MesosConfigure.cmake ce127e1e8c5c33ad8badef6fb3ac9f50ade9056f
Diff: https://reviews.apache.org/r/55607/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 55607: CMake: Added configuration of test scripts
in the bin/ directory.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55607/#review161838
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55022, 55023, 55024, 55030, 55037, 55038, 55039, 55040, 55161, 55162, 55311, 55312, 55313, 55314, 55327, 55328, 55543, 55544, 55546, 55547, 55548, 55549, 55550, 55599, 55600, 55601, 55602, 55604, 55607]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos ReviewBot
On Jan. 17, 2017, 8:34 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55607/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 8:34 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Bugs: MESOS-6757
> https://issues.apache.org/jira/browse/MESOS-6757
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This resolves MESOS-6757.
>
>
> Diffs
> -----
>
> cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc
>
> Diff: https://reviews.apache.org/r/55607/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55607: CMake: Added configuration of test scripts
in the bin/ directory.
Posted by Alex Clemmer <cl...@gmail.com>.
> On Jan. 25, 2017, 1:07 a.m., Joseph Wu wrote:
> > cmake/MesosConfigure.cmake, line 217
> > <https://reviews.apache.org/r/55607/diff/2/?file=1606454#file1606454line217>
> >
> > How about moving the file instead? And cleaning up the bin/tmp folder?
Unless I'm missing something, `COPY` is the preferred way to "move" a file, particularly if you want to set permissions. We can `REMOVE_RECURSE` to get rid of this `tmp/` folder though, and it should look basically identical to a move to a user.
> On Jan. 25, 2017, 1:07 a.m., Joseph Wu wrote:
> > cmake/MesosConfigure.cmake, line 212
> > <https://reviews.apache.org/r/55607/diff/2/?file=1606454#file1606454line212>
> >
> > This part ( https://cmake.org/cmake/help/v3.0/command/configure_file.html ): ```
> > If the <input> file is modified the build system will re-run CMake to re-configure the file and generate the build system again.
> > ```
> >
> > is a little unfortunate, but that's better than the automake, which doesn't always regenerate these template files when the underlying ones get changed.
It does suck, but I'm not sure what the alternatives are.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55607/#review162888
-----------------------------------------------------------
On Jan. 17, 2017, 8:34 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55607/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 8:34 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Bugs: MESOS-6757
> https://issues.apache.org/jira/browse/MESOS-6757
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This resolves MESOS-6757.
>
>
> Diffs
> -----
>
> cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc
>
> Diff: https://reviews.apache.org/r/55607/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55607: CMake: Added configuration of test scripts
in the bin/ directory.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55607/#review162888
-----------------------------------------------------------
cmake/MesosConfigure.cmake (line 212)
<https://reviews.apache.org/r/55607/#comment234269>
This part ( https://cmake.org/cmake/help/v3.0/command/configure_file.html ): ```
If the <input> file is modified the build system will re-run CMake to re-configure the file and generate the build system again.
```
is a little unfortunate, but that's better than the automake, which doesn't always regenerate these template files when the underlying ones get changed.
cmake/MesosConfigure.cmake (line 215)
<https://reviews.apache.org/r/55607/#comment234271>
Perhaps add a comment that `@ONLY` prevents us from substituting some value for `${@}`, which we commonly use in our template scripts.
cmake/MesosConfigure.cmake (line 217)
<https://reviews.apache.org/r/55607/#comment234270>
How about moving the file instead? And cleaning up the bin/tmp folder?
- Joseph Wu
On Jan. 17, 2017, 12:34 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55607/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 12:34 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Bugs: MESOS-6757
> https://issues.apache.org/jira/browse/MESOS-6757
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This resolves MESOS-6757.
>
>
> Diffs
> -----
>
> cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc
>
> Diff: https://reviews.apache.org/r/55607/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55607: CMake: Added configuration of test scripts
in the bin/ directory.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55607/
-----------------------------------------------------------
(Updated Jan. 17, 2017, 8:34 a.m.)
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Changes
-------
Fix typos in comments.
Bugs: MESOS-6757
https://issues.apache.org/jira/browse/MESOS-6757
Repository: mesos
Description
-------
This resolves MESOS-6757.
Diffs (updated)
-----
cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc
Diff: https://reviews.apache.org/r/55607/diff/
Testing
-------
Thanks,
Alex Clemmer