You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2019/03/01 00:02:41 UTC
Re: Review Request 70047: Updated build specific artefact generation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70047/#review213313
-----------------------------------------------------------
Fix it, then Ship it!
Thanks Till. While I am not a big fan of how we directly emit CPP macros from configure, I cannot think of a much better approach.
Left some mostly stylistic points.
cmake/CompilationConfigure.cmake
Lines 630-637 (original), 663-667 (patched)
<https://reviews.apache.org/r/70047/#comment299248>
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.
configure.ac
Lines 2829 (patched)
<https://reviews.apache.org/r/70047/#comment299243>
Lets use `presence` instead of `for`,
checking src/common/build_git_config.hpp presence ...
configure.ac
Lines 2831 (patched)
<https://reviews.apache.org/r/70047/#comment299245>
Even though there is some nesting I'd prefer more general `AS_IF` and friends.
Here and below.
configure.ac
Lines 2836 (patched)
<https://reviews.apache.org/r/70047/#comment299244>
`s/creating/generating/`
configure.ac
Lines 2843 (patched)
<https://reviews.apache.org/r/70047/#comment299246>
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.
src/common/build_git_config.hpp.in
Lines 1 (patched)
<https://reviews.apache.org/r/70047/#comment299242>
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?
src/common/build_git_config.hpp.in
Lines 23-25 (patched)
<https://reviews.apache.org/r/70047/#comment299247>
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 :(
- Benjamin Bannier
On Feb. 26, 2019, 12:05 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70047/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2019, 12:05 p.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
>
>
Re: Review Request 70047: Updated build specific artefact generation.
Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.
> 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
>
>