> On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote: > > cmake/CompilationConfigure.cmake > > Lines 630-637 (original), 663-667 (patched) > > <https://reviews.apache.org/r/70047/diff/4/?file=2126973#file2126973line663> > > > > The idea (at least on the autotools side which currently supports > > packaging) is to generate an artifact specifiying git info which is > > included with the distribution tarball. > > > > AFAICT, unless we completely rip out autotools while implementing cmake > > distribution tarballs, we'll need to implement something like this to > > support showing git info when building from distribution tarballs, even in > > the cmake build (which would probably emit this also during packaging time). > > > > Suggest to keep as is.
Yup! > On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote: > > configure.ac > > Lines 2829 (patched) > > <https://reviews.apache.org/r/70047/diff/4/?file=2126974#file2126974line2830> > > > > Lets use `presence` instead of `for`, > > > > checking src/common/build_git_config.hpp presence ... ok > On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote: > > configure.ac > > Lines 2831 (patched) > > <https://reviews.apache.org/r/70047/diff/4/?file=2126974#file2126974line2832> > > > > Even though there is some nesting I'd prefer more general `AS_IF` and > > friends. > > > > Here and below. ok > On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote: > > configure.ac > > Lines 2836 (patched) > > <https://reviews.apache.org/r/70047/diff/4/?file=2126974#file2126974line2837> > > > > `s/creating/generating/` ok > On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote: > > configure.ac > > Lines 2843 (patched) > > <https://reviews.apache.org/r/70047/diff/4/?file=2126974#file2126974line2844> > > > > Do you understand why this command could print something to stderr? It > > seems it should be possible to treat the absent case as error and define > > the variable with a value immediately. > > > > Also, what do you think about instead > > ``` > > git --git-dir=${srcdir}"/.git/ rev-parse HEAD ... > > ``` > > > > We could put the git dir into a variable. > > > > Here and below. > Do you understand why this command could print something to stderr? Yes - well not the head SHA, that should indeed always work. Let me take the tag for an example; ``` lobomacpro4:~/Development/mesos-private (ci/till/build-config-revamp ?) $ git describe --exact --tags fatal: no tag exactly matches '9ed7e160f003b5e22bf9c41e0104e0efca1df682' lobomacpro4:~/Development/mesos-private (ci/till/build-config-revamp ?) $ git describe --exact --tags 2>/dev/null lobomacpro4:~/Development/mesos-private (ci/till/build-config-revamp ?) $ echo $? 128 ``` Then imagine a detached checkout, that would similarly fail for the branch detection. ``` lobomacpro4:~/Development/mesos-private (no branch! ?) $ git symbolic-ref HEAD fatal: ref HEAD is not a symbolic ref lobomacpro4:~/Development/mesos-private (no branch! ?) $ git symbolic-ref HEAD 2>/dev/null lobomacpro4:~/Development/mesos-private (no branch! ?) $ echo $? 128 ``` So I would argue that the current approach for error handling is rather well adapted. > > `git --git-dir=${srcdir}"/.git/ rev-parse HEAD ...` Much better, thanks! > On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote: > > src/common/build_git_config.hpp.in > > Lines 1 (patched) > > <https://reviews.apache.org/r/70047/diff/4/?file=2126978#file2126978line1> > > > > Since this isn't about the build but git version, what do you think > > abot calling this file e.g., `git_version.hpp.in` or similar? Yes - good point. > On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote: > > src/common/build_git_config.hpp.in > > Lines 23-25 (patched) > > <https://reviews.apache.org/r/70047/diff/4/?file=2126978#file2126978line23> > > > > Not really a fan of how we directly emit a CPP macro from autoconf, but > > without config headers I cannot really think of a objectively better > > approach :( I wish you could! :( - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70047/#review213313 ----------------------------------------------------------- On Feb. 26, 2019, 11:05 a.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70047/ > ----------------------------------------------------------- > > (Updated Feb. 26, 2019, 11:05 a.m.) > > > Review request for mesos, Benjamin Bannier and Joseph Wu. > > > Bugs: MESOS-9605 > https://issues.apache.org/jira/browse/MESOS-9605 > > > Repository: mesos > > > Description > ------- > > For autotools, we extracted additional build info like the git branch > and sha during the automake phase handing them into libbuild via > commandline defines. > > CMake builds however used a configuration file for this purpose. > > This patch updates both build systems to make use of > build_git_config.hpp.in for build specific git information. > > > Diffs > ----- > > cmake/CompilationConfigure.cmake c330324e2e3dea6e71980ae8c9ed71632ebb018e > configure.ac ee29fc784e53ebaf1bb016c33136b200c646ee9b > src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261 > src/common/build.cpp f5271d87d33ac429fb94093a347be1d6c25d3432 > src/common/build_config.hpp.in 4cce2403c1d7a5feee8fd2fffa7cf4308507cd0c > src/common/build_git_config.hpp.in PRE-CREATION > > > Diff: https://reviews.apache.org/r/70047/diff/4/ > > > Testing > ------- > > Manually tested both cmake and autotools. > > First configure run: > ``` > [...] > checking for src/common/build_git_config.hpp... no > configure: creating src/common/build_git_config.hpp > [...] > > ``` > > Subsequent configure runs: > ``` > [...] > checking for src/common/build_git_config.hpp... yes > [...] > ``` > > Final build from `support/packaging/centos/build-docker-rpmbuild.sh` > installed and ran agent; > ``` > Installed: > mesos.x86_64 0:1.8.0-0.1.pre.20190226gitc125541.el7 > > Complete! > [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050 > I0226 01:00:48.581748 157 main.cpp:350] Build: 2019-02-26 00:36:49 by centos > I0226 01:00:48.581817 157 main.cpp:351] Version: 1.8.0 > I0226 01:00:48.581823 157 main.cpp:358] Git SHA: > c125541b8e4f2c2f6f56fe7e1c2e0b26d5bbfc62 > I0226 01:00:48.587003 157 systemd.cpp:240] systemd version `219` detected > I0226 01:00:48.587026 157 main.cpp:453] Initializing systemd state > ``` > > > Thanks, > > Till Toenshoff > >
