Re: Review Request 71240: Added a `dist` target to the cmake build.

2019-09-06 Thread Benjamin Bannier 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71240/
---

(Updated Sept. 6, 2019, 12:13 p.m.)


Review request for mesos, Benjamin Mahler and Till Toenshoff.


Repository: mesos


Description
---

This patch adds a `dist` target to the cmake build, analogous to the
target provided by the autotools build.

While cmake already provides a `package_source` target to create a
source archive it does not take care of only including relevant files,
but instead adds all files to the archive, e.g., including build or
backup files which should not be part of a release. For that reason this
patch introduces a script which performs a temporary `git clone` from
the checked out git repository and creates the archive from that clean
tree.

This patch also removes a hardcoded list of ignored files which was
by design not exhaustive.


Diffs (updated)
-

  CMakeLists.txt dc50dd457585c14a456ca710da4a16a0d45ef17a 
  cmake/dist.sh PRE-CREATION 


Diff: https://reviews.apache.org/r/71240/diff/3/

Changes: https://reviews.apache.org/r/71240/diff/2-3/


Testing
---

`ninja dist` produces a tarball, no artifacts left around but tarball.


Thanks,

Benjamin Bannier

Re: Review Request 71240: Added a `dist` target to the cmake build.

2019-09-05 Thread Till Toenshoff via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71240/#review217579
---




cmake/dist.sh
Lines 30 (patched)


Will fail with my setup as my repo-dir is called different than "mesos".


- Till Toenshoff


On Sept. 3, 2019, 1:52 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71240/
> ---
> 
> (Updated Sept. 3, 2019, 1:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a `dist` target to the cmake build, analogous to the
> target provided by the autotools build.
> 
> While cmake already provides a `package_source` target to create a
> source archive it does not take care of only including relevant files,
> but instead adds all files to the archive, e.g., including build or
> backup files which should not be part of a release. For that reason this
> patch introduces a script which performs a temporary `git clone` from
> the checked out git repository and creates the archive from that clean
> tree.
> 
> This patch also removes a hardcoded list of ignored files which was
> by design not exhaustive.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt dc50dd457585c14a456ca710da4a16a0d45ef17a 
>   cmake/dist.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71240/diff/2/
> 
> 
> Testing
> ---
> 
> `ninja dist` produces a tarball, no artifacts left around but tarball.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71240: Added a `dist` target to the cmake build.

2019-09-05 Thread Till Toenshoff via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71240/#review217578
---




cmake/dist.sh
Lines 37 (patched)


s/Sucessfully/Successfully/


- Till Toenshoff


On Sept. 3, 2019, 1:52 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71240/
> ---
> 
> (Updated Sept. 3, 2019, 1:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a `dist` target to the cmake build, analogous to the
> target provided by the autotools build.
> 
> While cmake already provides a `package_source` target to create a
> source archive it does not take care of only including relevant files,
> but instead adds all files to the archive, e.g., including build or
> backup files which should not be part of a release. For that reason this
> patch introduces a script which performs a temporary `git clone` from
> the checked out git repository and creates the archive from that clean
> tree.
> 
> This patch also removes a hardcoded list of ignored files which was
> by design not exhaustive.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt dc50dd457585c14a456ca710da4a16a0d45ef17a 
>   cmake/dist.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71240/diff/2/
> 
> 
> Testing
> ---
> 
> `ninja dist` produces a tarball, no artifacts left around but tarball.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71240: Added a `dist` target to the cmake build.

2019-09-05 Thread Till Toenshoff via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71240/#review217577
---


Ship it!




Ship It!

- Till Toenshoff


On Sept. 3, 2019, 1:52 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71240/
> ---
> 
> (Updated Sept. 3, 2019, 1:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a `dist` target to the cmake build, analogous to the
> target provided by the autotools build.
> 
> While cmake already provides a `package_source` target to create a
> source archive it does not take care of only including relevant files,
> but instead adds all files to the archive, e.g., including build or
> backup files which should not be part of a release. For that reason this
> patch introduces a script which performs a temporary `git clone` from
> the checked out git repository and creates the archive from that clean
> tree.
> 
> This patch also removes a hardcoded list of ignored files which was
> by design not exhaustive.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt dc50dd457585c14a456ca710da4a16a0d45ef17a 
>   cmake/dist.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71240/diff/2/
> 
> 
> Testing
> ---
> 
> `ninja dist` produces a tarball, no artifacts left around but tarball.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71240: Added a `dist` target to the cmake build.

2019-09-03 Thread Benjamin Bannier 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71240/#review217549
---



I compared the produced tarballs:

- 25 files only in autotools tarball:

mesos-1.10.0/3rdparty/libprocess/3rdparty/Makefile.in
mesos-1.10.0/3rdparty/libprocess/include/Makefile.in
mesos-1.10.0/3rdparty/libprocess/Makefile.in
mesos-1.10.0/3rdparty/Makefile.in
mesos-1.10.0/3rdparty/stout/3rdparty/Makefile.in
mesos-1.10.0/3rdparty/stout/include/Makefile.in
mesos-1.10.0/3rdparty/stout/Makefile.in
mesos-1.10.0/aclocal.m4
mesos-1.10.0/ar-lib
mesos-1.10.0/compile
mesos-1.10.0/config.guess
mesos-1.10.0/config.sub
mesos-1.10.0/configure
mesos-1.10.0/depcomp
mesos-1.10.0/install-sh
mesos-1.10.0/ltmain.sh
mesos-1.10.0/m4/libtool.m4
mesos-1.10.0/m4/lt~obsolete.m4
mesos-1.10.0/m4/ltoptions.m4
mesos-1.10.0/m4/ltsugar.m4
mesos-1.10.0/m4/ltversion.m4
mesos-1.10.0/Makefile.in
mesos-1.10.0/missing
mesos-1.10.0/src/common/git_version.hpp
mesos-1.10.0/src/Makefile.in


- 615 files only in cmake tarball:

mesos-1.10.0/3rdparty/boost.md
mesos-1.10.0/3rdparty/bzip2-1.0.6.tar.gz
mesos-1.10.0/3rdparty/cmake/
mesos-1.10.0/3rdparty/cmake/cotire.cmake
mesos-1.10.0/3rdparty/cmake/External.cmake
mesos-1.10.0/3rdparty/cmake/FindLEVELDB.cmake
mesos-1.10.0/3rdparty/cmake/FindLIBARCHIVE.cmake
mesos-1.10.0/3rdparty/cmake/FindLIBEVENT.cmake
mesos-1.10.0/3rdparty/cmake/FindLIBSECCOMP.cmake
mesos-1.10.0/3rdparty/CMakeLists.txt
mesos-1.10.0/3rdparty/cmake/PatchCommand.cmake
mesos-1.10.0/3rdparty/cmake/Versions.cmake
mesos-1.10.0/3rdparty/concurrentqueue.md
mesos-1.10.0/3rdparty/grpc.md
mesos-1.10.0/3rdparty/http-parser/
mesos-1.10.0/3rdparty/http-parser/CMakeLists.txt.template
mesos-1.10.0/3rdparty/libprocess/bootstrap
mesos-1.10.0/3rdparty/libprocess/CMakeLists.txt
mesos-1.10.0/3rdparty/libprocess/configure.ac
mesos-1.10.0/3rdparty/libprocess/examples/CMakeLists.txt
mesos-1.10.0/3rdparty/libprocess/include/process/windows/
mesos-1.10.0/3rdparty/libprocess/include/process/windows/jobobject.hpp
mesos-1.10.0/3rdparty/libprocess/LICENSE
mesos-1.10.0/3rdparty/libprocess/m4/
mesos-1.10.0/3rdparty/libprocess/m4/ax_append_compile_flags.m4
mesos-1.10.0/3rdparty/libprocess/m4/ax_append_flag.m4
mesos-1.10.0/3rdparty/libprocess/m4/ax_check_compile_flag.m4
mesos-1.10.0/3rdparty/libprocess/m4/ax_compare_version.m4
mesos-1.10.0/3rdparty/libprocess/m4/ax_compiler_vendor.m4
mesos-1.10.0/3rdparty/libprocess/m4/ax_compiler_version.m4
mesos-1.10.0/3rdparty/libprocess/m4/ax_cxx_compile_stdcxx.m4
mesos-1.10.0/3rdparty/libprocess/m4/ax_pthread.m4
mesos-1.10.0/3rdparty/libprocess/m4/ax_require_defined.m4
mesos-1.10.0/3rdparty/libprocess/m4/libevent.m4
mesos-1.10.0/3rdparty/libprocess/README.md
mesos-1.10.0/3rdparty/libprocess/src/CMakeLists.txt
mesos-1.10.0/3rdparty/libprocess/src/tests/CMakeLists.txt
mesos-1.10.0/3rdparty/libprocess/src/tests/test_echo.cpp
mesos-1.10.0/3rdparty/libprocess/src/windows/
mesos-1.10.0/3rdparty/libprocess/src/windows/event_loop.cpp
mesos-1.10.0/3rdparty/libprocess/src/windows/event_loop.hpp
mesos-1.10.0/3rdparty/libprocess/src/windows/io.cpp
mesos-1.10.0/3rdparty/libprocess/src/windows/libwinio.cpp
mesos-1.10.0/3rdparty/libprocess/src/windows/libwinio.hpp
mesos-1.10.0/3rdparty/libprocess/src/windows/poll_socket.cpp
mesos-1.10.0/3rdparty/libprocess/src/windows/subprocess.cpp
mesos-1.10.0/3rdparty/libprocess/src/windows/subprocess.hpp
mesos-1.10.0/3rdparty/patch.exe.manifest
mesos-1.10.0/3rdparty/protobuf.md
mesos-1.10.0/3rdparty/rapidjson.md
mesos-1.10.0/3rdparty/README.md
mesos-1.10.0/3rdparty/stout/bootstrap
mesos-1.10.0/3rdparty/stout/cmake/
mesos-1.10.0/3rdparty/stout/cmake/FindAPR.cmake
mesos-1.10.0/3rdparty/stout/cmake/FindPackageHelper.cmake
mesos-1.10.0/3rdparty/stout/cmake/FindSVN.cmake
mesos-1.10.0/3rdparty/stout/CMakeLists.txt
mesos-1.10.0/3rdparty/stout/configure.ac
mesos-1.10.0/3rdparty/stout/include/stout/internal/windows/inherit.hpp
mesos-1.10.0/3rdparty/stout/include/stout/os/sunos.hpp
mesos-1.10.0/3rdparty/stout/LICENSE
mesos-1.10.0/3rdparty/stout/m4/
mesos-1.10.0/3rdparty/stout/m4/ax_append_compile_flags.m4
mesos-1.10.0/3rdparty/stout/m4/ax_append_flag.m4
mesos-1.10.0/3rdparty/stout/m4/ax_check_compile_flag.m4
mesos-1.10.0/3rdparty/stout/m4/ax_compare_version.m4
mesos-1.10.0/3rdparty/stout/m4/ax_compiler_vendor.m4
mesos-1.10.0/3rdparty/stout/m4/ax_compiler_version.m4
mesos-1.10.0/3rdparty/stout/m4/ax_cxx_compile_stdcxx.m4
mesos-1.10.0/3rdparty/stout/m4/ax_pthread.m4
mesos-1.10.0/3rdparty/stout/m4/ax_require_defined

Re: Review Request 71240: Added a `dist` target to the cmake build.

2019-09-03 Thread Benjamin Bannier 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71240/
---

(Updated Sept. 3, 2019, 3:52 p.m.)


Review request for mesos, Benjamin Mahler and Till Toenshoff.


Changes
---

Address issues raised by Benno


Repository: mesos


Description
---

This patch adds a `dist` target to the cmake build, analogous to the
target provided by the autotools build.

While cmake already provides a `package_source` target to create a
source archive it does not take care of only including relevant files,
but instead adds all files to the archive, e.g., including build or
backup files which should not be part of a release. For that reason this
patch introduces a script which performs a temporary `git clone` from
the checked out git repository and creates the archive from that clean
tree.

This patch also removes a hardcoded list of ignored files which was
by design not exhaustive.


Diffs (updated)
-

  CMakeLists.txt dc50dd457585c14a456ca710da4a16a0d45ef17a 
  cmake/dist.sh PRE-CREATION 


Diff: https://reviews.apache.org/r/71240/diff/2/

Changes: https://reviews.apache.org/r/71240/diff/1-2/


Testing
---

`ninja dist` produces a tarball, no artifacts left around but tarball.


Thanks,

Benjamin Bannier

Re: Review Request 71240: Added a `dist` target to the cmake build.

2019-09-03 Thread Benjamin Bannier 


> On Sept. 3, 2019, 3:08 p.m., Benno Evers wrote:
> > cmake/dist.sh
> > Lines 8 (patched)
> > 
> >
> > Why this restriction if we're going to create the tarball from a clean 
> > checkout anyways?

The purpose of this is to prevent situations where somebody creates a dist 
tarball from a dirty tree only to not have the changes reflected in the created 
tarball. This would be confusing and error-prone behavior.


> On Sept. 3, 2019, 3:08 p.m., Benno Evers wrote:
> > cmake/dist.sh
> > Lines 19 (patched)
> > 
> >
> > I'm not sure if this is written down anywhere, but in general it's a 
> > good shell-script best practice to use `SCREAMING_CASE` for variables that 
> > can be overriden from outside and `lower_case` for variables that are 
> > internal to the script.

That sounds reasonable, but would not at all be in line with our current 
convention.

Dropping this for now.


> On Sept. 3, 2019, 3:08 p.m., Benno Evers wrote:
> > cmake/dist.sh
> > Lines 21 (patched)
> > 
> >
> > Can we find a more speaking name than `D` for this? Also, since this is 
> > used as a temporary directory it might make sense to use `mktemp -d` rather 
> > than hardcoding the directory name.

Great suggestion on the temp dir (I had modelled this after autotools' `make 
distcheck` always using a fixed output directory in the build directory; now 
that I think about that I am not sure that was ever useful to me and not just 
an opportunity for name collisions in a dirty tree). Went with `WORKDIR` as var 
name.


> On Sept. 3, 2019, 3:08 p.m., Benno Evers wrote:
> > cmake/dist.sh
> > Lines 29 (patched)
> > 
> >
> > Can you explain why in-tree builds are not supported, i.e. why do we 
> > need to create a separate build directory here?

TBH, I cannot, but this is a general feature of our cmake setup (I suspect it 
has to do with aspects like generated files).

Dropping.


> On Sept. 3, 2019, 3:08 p.m., Benno Evers wrote:
> > cmake/dist.sh
> > Lines 33 (patched)
> > 
> >
> > I assume `nproc` is some magic cmake variable that is set automatically?

`nproc` is part of coreutils which we also assume elsewhere.


- Benjamin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71240/#review217539
---


On Sept. 3, 2019, 3:52 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71240/
> ---
> 
> (Updated Sept. 3, 2019, 3:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a `dist` target to the cmake build, analogous to the
> target provided by the autotools build.
> 
> While cmake already provides a `package_source` target to create a
> source archive it does not take care of only including relevant files,
> but instead adds all files to the archive, e.g., including build or
> backup files which should not be part of a release. For that reason this
> patch introduces a script which performs a temporary `git clone` from
> the checked out git repository and creates the archive from that clean
> tree.
> 
> This patch also removes a hardcoded list of ignored files which was
> by design not exhaustive.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt dc50dd457585c14a456ca710da4a16a0d45ef17a 
>   cmake/dist.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71240/diff/2/
> 
> 
> Testing
> ---
> 
> `ninja dist` produces a tarball, no artifacts left around but tarball.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71240: Added a `dist` target to the cmake build.

2019-09-03 Thread Benno Evers 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71240/#review217539
---




cmake/dist.sh
Lines 8 (patched)


Why this restriction if we're going to create the tarball from a clean 
checkout anyways?



cmake/dist.sh
Lines 19 (patched)


I'm not sure if this is written down anywhere, but in general it's a good 
shell-script best practice to use `SCREAMING_CASE` for variables that can be 
overriden from outside and `lower_case` for variables that are internal to the 
script.



cmake/dist.sh
Lines 21 (patched)


Can we find a more speaking name than `D` for this? Also, since this is 
used as a temporary directory it might make sense to use `mktemp -d` rather 
than hardcoding the directory name.



cmake/dist.sh
Lines 29 (patched)


Can you explain why in-tree builds are not supported, i.e. why do we need 
to create a separate build directory here?



cmake/dist.sh
Lines 33 (patched)


I assume `nproc` is some magic cmake variable that is set automatically?


- Benno Evers


On Aug. 6, 2019, 7:28 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71240/
> ---
> 
> (Updated Aug. 6, 2019, 7:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a `dist` target to the cmake build, analogous to the
> target provided by the autotools build.
> 
> While cmake already provides a `package_source` target to create a
> source archive it does not take care of only including relevant files,
> but instead adds all files to the archive, e.g., including build or
> backup files which should not be part of a release. For that reason this
> patch introduces a script which performs a temporary `git clone` from
> the checked out git repository and creates the archive from that clean
> tree.
> 
> This patch also removes a hardcoded list of ignored files which was
> by design not exhaustive.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 65bfbac0e2b6aec26a6d98ff96615661210ceac2 
>   cmake/dist.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71240/diff/1/
> 
> 
> Testing
> ---
> 
> `ninja dist` produces a tarball, no artifacts left around but tarball.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 71240: Added a `dist` target to the cmake build.

2019-08-06 Thread Benjamin Bannier 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71240/
---

Review request for mesos, Benjamin Mahler and Till Toenshoff.


Repository: mesos


Description
---

This patch adds a `dist` target to the cmake build, analogous to the
target provided by the autotools build.

While cmake already provides a `package_source` target to create a
source archive it does not take care of only including relevant files,
but instead adds all files to the archive, e.g., including build or
backup files which should not be part of a release. For that reason this
patch introduces a script which performs a temporary `git clone` from
the checked out git repository and creates the archive from that clean
tree.

This patch also removes a hardcoded list of ignored files which was
by design not exhaustive.


Diffs
-

  CMakeLists.txt 65bfbac0e2b6aec26a6d98ff96615661210ceac2 
  cmake/dist.sh PRE-CREATION 


Diff: https://reviews.apache.org/r/71240/diff/1/


Testing
---

`ninja dist` produces a tarball, no artifacts left around but tarball.


Thanks,

Benjamin Bannier