Re: [PATCH v6 6/6] Add Travis CI support

From: Jeff King <peff@peff.net>
Date: Nov 19, 2015, 14:35 UTC
Message-ID: <20151119143528.GC9353@sigill.intra.peff.net>
In-Reply-To: <1447923491-15330-7-git-send-email-larsxschneider@gmail.com>

On Thu, Nov 19, 2015 at 09:58:11AM +0100, larsxschneider@gmail.com wrote:

Show 7 quoted lines

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> The tests are currently executed on "Ubuntu 12.04 LTS Server Edition
> 64 bit" and on "OS X Mavericks" using gcc and clang.
> 
> Perforce and Git-LFS are installed and therefore available for the
> respective tests.

My overall impression is that this is a lot more complicated than I was expecting. Can we start with something simpler to gain experience with Travis? And then we can add in more complexity later.

For example:

> +addons:
> +  apt:
> +    packages:
> +    - language-pack-is

I doubt most people are running the t0204 right now. Maybe we should start without it.

> +env:
> +  global:
> +    - P4_VERSION="15.1"
> +    - GIT_LFS_VERSION="1.0.2"

I know p4 is your area of interest, but from a project perspective, it seems like an odd choice for the initial set of tests.

> +    - DEFAULT_TEST_TARGET=prove
> +    - GIT_PROVE_OPTS="--timer --jobs 3"
> +    - GIT_TEST_OPTS="--verbose --tee"

These all make sense, I guess.

> +    - GETTEXT_ISO_LOCALE=YesPlease
> +    - GETTEXT_LOCALE=YesPlease

What are these? I don't think we have any such Makefile knobs. These look like variables that get used internally by the test suite, but we shouldn't need to set them.

> +    # - GETTEXT_POISON=YesPlease

I'm assuming the "#" means this is commented out. Can we just drop such cruft?

> +    - GIT_TEST_CHAIN_LINT=YesPlease

This is the default, and we don't need to specify it.

> +    - GIT_TEST_CLONE_2GB=YesPlease

Is it a good idea to run such an expensive test?

> +  matrix:
> +    -
> +      # NO_ICONV=YesPlease

Ditto here on the commenting.

> +    - >
> +      NO_CURL=YesPlease

So if I understand correctly, the point of this list is to test alternate configurations. So compiling without curl makes some sense, I guess. Though mostly it will just shut off a bunch off tests.

> +      NO_D_INO_IN_DIRENT=YesPlease

But setting platform compat knobs like this seems weird. Nobody sets this manually. Either your platform has it, or it does not. And we are already testing on alternate platforms to cover such things.

If there's a specific config setup of interest, it makes sense to me to try to increase test coverage there. But it feels like you've just picked a random laundry list of Makefile knobs and tweaked them, without any sense of whether they even make sense together, or match the platform.

For instance:

> +      NO_STRCASESTR=YesPlease
> +      NO_STRLCPY=YesPlease

I wouldn't not be surprised if these cause problems building on some platforms that _do_ have these functions.

> +    echo "$(tput setaf 6)Perfoce Server Version$(tput sgr0)";
> +    p4d -V | grep Rev.;
> +    echo "$(tput setaf 6)Perfoce Client Version$(tput sgr0)";
> +    p4 -V | grep Rev.;

s/Perfoce/Perforce/ :)

> +before_script: make configure && ./configure && make --jobs=2

Hmm. I wonder if we actually want to use autoconf here at all; most devs do not use it, and Git should generally compile out of the box based on config.mak.uname (and if it doesn't, it would be nice to know).

There may be some optional packages that autoconf helps with, though.

So overall, I dunno. I do not want to create a bunch of work for you in splitting it up. But I'm hesitant to merge something that has a bunch of lines that I'm not sure I understand the reasoning for. :-/

I was hoping the first cut could just be telling Travis to run "make test", without much fanfare beyond that. Maybe that's not realistic.

-Peff