From: Johannes Schindelin <hidden> Date: 2017-02-28 19:37:46
When Git v2.9.1 was released, it had a bug that showed only on Windows
and on 32-bit systems: our assumption that `unsigned long` can hold
64-bit values turned out to be wrong.
This could have been caught earlier if we had a Continuous Testing
set up that includes a build and test run on 32-bit Linux.
Let's do this (and take care of the Windows build later). This patch
asks Travis CI to install a Docker image with 32-bit libraries and then
goes on to build and test Git using this 32-bit setup.
A big thank you to Lars Schneider without whose help this patch would
not have happened.
Signed-off-by: Johannes Schindelin <redacted>
---
Published-As: https://github.com/dscho/git/releases/tag/travis-32-bit-v1
Fetch-It-Via: git fetch https://github.com/dscho/git travis-32-bit-v1
.travis.yml | 11 +++++++++++
1 file changed, 11 insertions(+)
From: Johannes Schindelin <hidden> Date: 2017-02-28 20:36:57
Hi,
On Tue, 28 Feb 2017, Johannes Schindelin wrote:
When Git v2.9.1 was released, it had a bug that showed only on Windows
and on 32-bit systems: our assumption that `unsigned long` can hold
64-bit values turned out to be wrong.
This could have been caught earlier if we had a Continuous Testing set
up that includes a build and test run on 32-bit Linux.
Let's do this (and take care of the Windows build later). This patch
asks Travis CI to install a Docker image with 32-bit libraries and then
goes on to build and test Git using this 32-bit setup.
For the record, I first tested this with the LONG_IS_32BIT flag forced to
`true` so that the date tests must fail. They did fail as expected (search
for "not ok" in this output):
https://travis-ci.org/git/git/jobs/206199002
(I actually cannot see the log right now, Travis seems to be under heavy
load...)
A much cleaned up version that does not force that LONG_IS_32BIT produced
this log:
https://travis-ci.org/git/git/jobs/206228708
(Same here, my browser fails to load the log, probably because Travis
experiences quite high a load...)
Please note that this approach is not without problems. It would appear
that Travis currently has serious problems to even *reach* the Docker Hub,
and therefore cannot download the Docker image:
https://travis-ci.org/git/git/jobs/206292947#L6
Ciao,
Johannes
From: Junio C Hamano <hidden> Date: 2017-03-02 05:06:52
On Tue, Feb 28, 2017 at 11:17 AM, Johannes Schindelin
[off-list ref] wrote:
.... This patch
asks Travis CI to install a Docker image with 32-bit libraries and then
goes on to build and test Git using this 32-bit setup.
A big thank you to Lars Schneider without whose help this patch would
not have happened.
This has been in 'pu' for a few days, and
https://travis-ci.org/git/git/builds shows that we have
a new build job running successfully.
Good job ;-)
Thanks.
From: Lars Schneider <hidden> Date: 2017-03-02 10:59:52
From: Johannes Schindelin <redacted>
When Git v2.9.1 was released, it had a bug that showed only on Windows
and on 32-bit systems: our assumption that `unsigned long` can hold
64-bit values turned out to be wrong.
This could have been caught earlier if we had a Continuous Testing
set up that includes a build and test run on 32-bit Linux.
Let's do this (and take care of the Windows build later). This patch
asks Travis CI to install a Docker image with 32-bit libraries and then
goes on to build and test Git using this 32-bit setup.
Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Lars Schneider <redacted>
---
Thanks for the patch Dscho!
The patch looks good to me in general but I want to propose the following
changes:
(1) Move all the docker magic into a dedicated file "ci/run-linux-32-build.sh"
This way people should be able to run this build on their local machines
without TravisCI. However, I haven't tested this.
(2) The docker build command inherits the Git test environment variables.
This way we use the same environment variables as in all other TravisCI
builds (plus it would use *your* variables if you run it locally).
(3) Silence the apt update/git output as is it clutters the log.
I did not silence stderr output!
(4) Remove (to my knowledge) superfluous "compiler: clang" in the Linux32 job.
I added my sign-off. I hope this is the right thing to do in this "I took your
patch and changed it to suggest an improvement" situation.
You can see a successful run here:
https://travis-ci.org/larsxschneider/git/jobs/206945950
One thing that still bugs me: In the Linux32 environment prove adds the
CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00 csys ...
Has anyone an idea why that happens and how we can disable it?
Cheers,
Lars
Notes:
Base Ref:
Web-Diff: https://github.com/larsxschneider/git/commit/82995ed59c
Checkout: git fetch https://github.com/larsxschneider/git travisci/linux32-v1 && git checkout 82995ed59c
.travis.yml | 9 +++++++++
ci/run-linux32-build.sh | 21 +++++++++++++++++++++
2 files changed, 30 insertions(+)
create mode 100755 ci/run-linux32-build.sh
From: Johannes Schindelin <hidden> Date: 2017-03-02 12:19:45
Hi Lars,
On Thu, 2 Mar 2017, Lars Schneider wrote:
The patch looks good to me in general but I want to propose the following
changes:
I know you are using your script to generate this mail, but I would have
liked to see v2 in the subject ;-)
(1) Move all the docker magic into a dedicated file
"ci/run-linux-32-build.sh" This way people should be able to run this
build on their local machines without TravisCI. However, I haven't
tested this.
I considered this, but there is serious overlap: the `docker pull` call
and the `docker run` call *have* to refer to the same image. It's very
easy for them to get out of sync if you have that information in two
files. Maybe make that an option of the script, defaulting to
daald/ubuntu32:xenial?
BTW speaking of Docker: it would be nicer if there was a Docker image that
already had the build-essentials installed, to save on startup time. But I
did not find any that was reasonably up-to-date.
(2) The docker build command inherits the Git test environment
variables. This way we use the same environment variables as in all
other TravisCI builds (plus it would use *your* variables if you run it
locally).
Good!
(3) Silence the apt update/git output as is it clutters the log. I did
not silence stderr output!
Also good!
(4) Remove (to my knowledge) superfluous "compiler: clang" in the
Linux32 job.
I copied that from one of your experimental .travis.yml patches ;-)
I added my sign-off. I hope this is the right thing to do in this "I
took your patch and changed it to suggest an improvement" situation.
Absolutely. Thank you for taking it from here.
One thing that still bugs me: In the Linux32 environment prove adds the
CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00
csys ... Has anyone an idea why that happens and how we can disable it?
Hmm. Since it is a script now, it would be more readable this way, I
think:
sudo docker run --volume "${PWD}:/usr/src/git" "${1:-daald/ubuntu32:xenial}" \
linux32 --32bit i386 sh -c '
: update packages first &&
apt update >/dev/null &&
apt install -y build-essential libcurl4-openssl-dev libssl-dev \
libexpat-dev gettext python >/dev/null &&
: now build and test &&
cd /usr/src/git &&
DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
make -j2 test
'
This is completely untested (pun intended ;-))...
Ciao,
Dscho
From: Lars Schneider <hidden> Date: 2017-03-02 14:52:30
On 02 Mar 2017, at 12:24, Johannes Schindelin [off-list ref] wrote:
Hi Lars,
On Thu, 2 Mar 2017, Lars Schneider wrote:
quoted
The patch looks good to me in general but I want to propose the following
changes:
I know you are using your script to generate this mail, but I would have
liked to see v2 in the subject ;-)
Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in
my email client. Now I am wondering... is the next version v2 or v3 :D
quoted
(1) Move all the docker magic into a dedicated file
"ci/run-linux-32-build.sh" This way people should be able to run this
build on their local machines without TravisCI. However, I haven't
tested this.
I considered this, but there is serious overlap: the `docker pull` call
and the `docker run` call *have* to refer to the same image. It's very
easy for them to get out of sync if you have that information in two
files. Maybe make that an option of the script, defaulting to
daald/ubuntu32:xenial?
Right. I missed that. How about something like that?
before_install:
- ci/run-linux32-build.sh --pull-container
before_script:
script: ci/run-linux32-build.sh
BTW speaking of Docker: it would be nicer if there was a Docker image that
already had the build-essentials installed, to save on startup time. But I
did not find any that was reasonably up-to-date.
True. But installing everything just takes a minute and we don't need to
maintain anything...
quoted
+set -e
Is this really necessary? I really like to avoid `set -e`, in particular
when we do pretty much everything in && chains anyway.
Agreed, not really necessary here as we just invoke one command.
Out of curiosity: Why do you try to avoid it? I set it by default in all
my scripts.
Hmm. Since it is a script now, it would be more readable this way, I
think:
sudo docker run --volume "${PWD}:/usr/src/git" "${1:-daald/ubuntu32:xenial}" \
linux32 --32bit i386 sh -c '
: update packages first &&
apt update >/dev/null &&
apt install -y build-essential libcurl4-openssl-dev libssl-dev \
libexpat-dev gettext python >/dev/null &&
: now build and test &&
cd /usr/src/git &&
DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
make -j2 test
'
From: Johannes Schindelin <hidden> Date: 2017-03-02 14:56:23
Hi Lars,
On Thu, 2 Mar 2017, Lars Schneider wrote:
quoted
On 02 Mar 2017, at 12:24, Johannes Schindelin [off-list ref] wrote:
On Thu, 2 Mar 2017, Lars Schneider wrote:
quoted
The patch looks good to me in general but I want to propose the
following changes:
I know you are using your script to generate this mail, but I would
have liked to see v2 in the subject ;-)
Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in my
email client. Now I am wondering... is the next version v2 or v3 :D
Since there was no v2, the next one should *definitely* be v2... ;-)
quoted
quoted
(1) Move all the docker magic into a dedicated file
"ci/run-linux-32-build.sh" This way people should be able to run this
build on their local machines without TravisCI. However, I haven't
tested this.
I considered this, but there is serious overlap: the `docker pull`
call and the `docker run` call *have* to refer to the same image. It's
very easy for them to get out of sync if you have that information in
two files. Maybe make that an option of the script, defaulting to
daald/ubuntu32:xenial?
Right. I missed that. How about something like that?
before_install:
- ci/run-linux32-build.sh --pull-container
before_script:
script: ci/run-linux32-build.sh
BTW speaking of Docker: it would be nicer if there was a Docker image
that already had the build-essentials installed, to save on startup
time. But I did not find any that was reasonably up-to-date.
True. But installing everything just takes a minute and we don't need to
maintain anything...
And when there are network problems (like there were on Tuesday, right
when I developed the first v1 of this patch) then we have another set of
problems that make Travis fail. Even if the code in the PR or branch is
actually good. I'd like to avoid false positives, if possible.
quoted
quoted
+set -e
Is this really necessary? I really like to avoid `set -e`, in
particular when we do pretty much everything in && chains anyway.
Agreed, not really necessary here as we just invoke one command. Out of
curiosity: Why do you try to avoid it? I set it by default in all my
scripts.
I try to avoid it because it encourages a style that omits helpful error
messages.
Hmm. Since it is a script now, it would be more readable this way, I
think:
sudo docker run --volume "${PWD}:/usr/src/git" "${1:-daald/ubuntu32:xenial}" \
linux32 --32bit i386 sh -c '
: update packages first &&
apt update >/dev/null &&
apt install -y build-essential libcurl4-openssl-dev libssl-dev \
libexpat-dev gettext python >/dev/null &&
: now build and test &&
cd /usr/src/git &&
DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
make -j2 test
'
From: Ramsay Jones <hidden> Date: 2017-03-02 15:33:20
On 02/03/17 11:24, Johannes Schindelin wrote:
Hi Lars,
On Thu, 2 Mar 2017, Lars Schneider wrote:
[snip]
quoted
One thing that still bugs me: In the Linux32 environment prove adds the
CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00
csys ... Has anyone an idea why that happens and how we can disable it?
I have no idea.
I have no idea either, but it is not unique to this 32bit Linux, but
rather the version of prove. For example, I am seeing this on Linux
Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do
this. (They used different Ubuntu LTS releases).
[Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1]
ATB,
Ramsay Jones
From: Lars Schneider <hidden> Date: 2017-03-05 17:45:20
On 02 Mar 2017, at 16:17, Ramsay Jones [off-list ref] wrote:
On 02/03/17 11:24, Johannes Schindelin wrote:
quoted
Hi Lars,
On Thu, 2 Mar 2017, Lars Schneider wrote:
[snip]
quoted
quoted
One thing that still bugs me: In the Linux32 environment prove adds the
CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00
csys ... Has anyone an idea why that happens and how we can disable it?
I have no idea.
I have no idea either, but it is not unique to this 32bit Linux, but
rather the version of prove. For example, I am seeing this on Linux
Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do
this. (They used different Ubuntu LTS releases).
[Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1]
From: Ramsay Jones <hidden> Date: 2017-03-05 22:16:37
On 05/03/17 17:38, Lars Schneider wrote:
quoted
On 02 Mar 2017, at 16:17, Ramsay Jones [off-list ref] wrote:
On 02/03/17 11:24, Johannes Schindelin wrote:
quoted
On Thu, 2 Mar 2017, Lars Schneider wrote:
[snip]
quoted
quoted
One thing that still bugs me: In the Linux32 environment prove adds the
CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00
csys ... Has anyone an idea why that happens and how we can disable it?
I have no idea.
I have no idea either, but it is not unique to this 32bit Linux, but
rather the version of prove. For example, I am seeing this on Linux
Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do
this. (They used different Ubuntu LTS releases).
[Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1]
Yep, that looks like it.
When I updated to Mint 18, this broke a perl script of mine, so I had
a quick look to see what I could do to suppress it. The man page seemed
to imply that you could replace the output formatter, but that didn't
take me too far (search CPAN for TAP::Parser::Formatter: ;-) ). I suppose
you could replace Tap::Formatter::Base, or some such, but I didn't need
to go that far - I simply changed a couple of regex-es to ignore the
excess output! :-P
Do you really need to suppress that timing information or, like me, can
you simply ignore it?
ATB,
Ramsay Jones