> On Sept. 3, 2019, 3:08 p.m., Benno Evers wrote: > > cmake/dist.sh > > Lines 8 (patched) > > <https://reviews.apache.org/r/71240/diff/1/?file=2159993#file2159993line8> > > > > 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) > > <https://reviews.apache.org/r/71240/diff/1/?file=2159993#file2159993line19> > > > > 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) > > <https://reviews.apache.org/r/71240/diff/1/?file=2159993#file2159993line21> > > > > 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) > > <https://reviews.apache.org/r/71240/diff/1/?file=2159993#file2159993line29> > > > > 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) > > <https://reviews.apache.org/r/71240/diff/1/?file=2159993#file2159993line33> > > > > 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 > >
