[libvirt] [PATCH 0/4] Misc travis improvements
Various improvements to travis coverage. The first patch was posted separately yesterday too. Daniel P. Berrangé (4): travis: add a scenario for running make distcheck travis: make all builds use VPATH travis: test upstart script handling on precise distro scenario travis: run 'make install' during build tests .travis.yml | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) -- 2.14.3
Running "make distcheck" ensures that we have CLEANFILES and uninstall rules setup correctly, as well as validating VPATH builds succeeed. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.travis.yml b/.travis.yml index 3f26a1eeee..4bdf034829 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,8 +6,13 @@ matrix: include: - compiler: gcc dist: precise + # Special scenario to run distcheck, so we don't waste time duplicating + # work in all the other scenarios. Doesn't work on precise due to the + # CVE-2012-3386 flaw being present on that Ubuntu version - compiler: gcc dist: trusty + script: + - make -j3 distcheck - compiler: clang dist: precise - compiler: clang -- 2.14.3
On Fri, 2018-02-23 at 12:00 +0000, Daniel P. Berrangé wrote:
Running "make distcheck" ensures that we have CLEANFILES and uninstall rules setup correctly, as well as validating VPATH builds succeeed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/.travis.yml b/.travis.yml index 3f26a1eeee..4bdf034829 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,8 +6,13 @@ matrix: include: - compiler: gcc dist: precise + # Special scenario to run distcheck, so we don't waste time duplicating + # work in all the other scenarios. Doesn't work on precise due to the + # CVE-2012-3386 flaw being present on that Ubuntu version - compiler: gcc dist: trusty + script: + - make -j3 distcheck - compiler: clang dist: precise - compiler: clang
This will override the default script, and make it so the precise/gcc build only runs distcheck rather than the usual all, check, syntax-check. So we need something else. My first idea was to have something like matrix: include: - compiler: gcc dist: precise env: - OS=precise - compiler: gcc dist: trusty env: - OS=trusty - compiler: clang dist: precise env: - OS=precise - compiler: clang dist: trusty env: - OS=trusty - compiler: clang os: osx env: - OS=macos script: - make -j3 && if [ "$OS" != "macos" ]; then make -j3 syntax-check && make -j3 check; fi && if [ "$OS" = "trusty" ]; then make -j3 distcheck; fi with the new env variable being introduced because, for whatever reason, Travis defines $TRAVIS_OS_NAME for telling Linux and macOS apart but no equivalent for telling precise and trusty apart :/ However, I've later realized that precise has already been EOL'd for almost a year and is already in the process of being (slowly) decommissioned on Travis too. So I say just rip out the damn obsolete thing already and move on with our lives :) -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Feb 27, 2018 at 04:02:19PM +0100, Andrea Bolognani wrote:
On Fri, 2018-02-23 at 12:00 +0000, Daniel P. Berrangé wrote:
Running "make distcheck" ensures that we have CLEANFILES and uninstall rules setup correctly, as well as validating VPATH builds succeeed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/.travis.yml b/.travis.yml index 3f26a1eeee..4bdf034829 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,8 +6,13 @@ matrix: include: - compiler: gcc dist: precise + # Special scenario to run distcheck, so we don't waste time duplicating + # work in all the other scenarios. Doesn't work on precise due to the + # CVE-2012-3386 flaw being present on that Ubuntu version - compiler: gcc dist: trusty + script: + - make -j3 distcheck - compiler: clang dist: precise - compiler: clang
This will override the default script, and make it so the precise/gcc build only runs distcheck rather than the usual all, check, syntax-check. So we need something else.
Yes, that's intentional and not a problem IMHO. 'check' is run as part of 'distcheck' so that's a non-issue. Running syntax-check in all 5 scenarios isn't buying us anything, as the syntax-check rules don't depend on what is installed in the host. IOW, running syntax-check in 1 scenario is sufficient to get us the coverage we need. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 2018-02-27 at 15:06 +0000, Daniel P. Berrangé wrote:
- compiler: gcc dist: precise + # Special scenario to run distcheck, so we don't waste time duplicating + # work in all the other scenarios. Doesn't work on precise due to the + # CVE-2012-3386 flaw being present on that Ubuntu version - compiler: gcc dist: trusty + script: + - make -j3 distcheck
This will override the default script, and make it so the precise/gcc build only runs distcheck rather than the usual all, check, syntax-check. So we need something else.
Yes, that's intentional and not a problem IMHO.
'check' is run as part of 'distcheck' so that's a non-issue.
Running syntax-check in all 5 scenarios isn't buying us anything, as the syntax-check rules don't depend on what is installed in the host. IOW, running syntax-check in 1 scenario is sufficient to get us the coverage we need.
Okay, fair enough. The change still "obfuscates" the Travis configuration though, because now you can't just look at a single script entry but you have to explode the matrix in your head and convince yourself you're covering all bases, so I'm not too happy with it. Moreover, there was a whole thing about just dropping support for precise (as Canonical already did) and making our lives easier later in the mail, but you snipped it without replying... -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Feb 27, 2018 at 04:35:32PM +0100, Andrea Bolognani wrote:
On Tue, 2018-02-27 at 15:06 +0000, Daniel P. Berrangé wrote:
- compiler: gcc dist: precise + # Special scenario to run distcheck, so we don't waste time duplicating + # work in all the other scenarios. Doesn't work on precise due to the + # CVE-2012-3386 flaw being present on that Ubuntu version - compiler: gcc dist: trusty + script: + - make -j3 distcheck
This will override the default script, and make it so the precise/gcc build only runs distcheck rather than the usual all, check, syntax-check. So we need something else.
Yes, that's intentional and not a problem IMHO.
'check' is run as part of 'distcheck' so that's a non-issue.
Running syntax-check in all 5 scenarios isn't buying us anything, as the syntax-check rules don't depend on what is installed in the host. IOW, running syntax-check in 1 scenario is sufficient to get us the coverage we need.
Okay, fair enough. The change still "obfuscates" the Travis configuration though, because now you can't just look at a single script entry but you have to explode the matrix in your head and convince yourself you're covering all bases, so I'm not too happy with it.
I don't think we've got so many different scenarios here that understanding it is a real problem
Moreover, there was a whole thing about just dropping support for precise (as Canonical already did) and making our lives easier later in the mail, but you snipped it without replying...
Opps, I'm not in favour of dropping precise, because I think it is useful to get coverage on older distros. Travis is what I use for testing complex patch series before submission, so I like it to have a useful mix of vintage OSs, not only the very latest that is largely the same as what I build on locally already. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 2018-02-27 at 15:41 +0000, Daniel P. Berrangé wrote:
Running syntax-check in all 5 scenarios isn't buying us anything, as the syntax-check rules don't depend on what is installed in the host. IOW, running syntax-check in 1 scenario is sufficient to get us the coverage we need.
Okay, fair enough. The change still "obfuscates" the Travis configuration though, because now you can't just look at a single script entry but you have to explode the matrix in your head and convince yourself you're covering all bases, so I'm not too happy with it.
I don't think we've got so many different scenarios here that understanding it is a real problem
It's not a massive hurdle, but it's still cognitive load that I'd rather not have to take on. See my first reply for a way of achieving the same result in a much more explicit and easy to grasp manner.
Moreover, there was a whole thing about just dropping support for precise (as Canonical already did) and making our lives easier later in the mail, but you snipped it without replying...
Opps, I'm not in favour of dropping precise, because I think it is useful to get coverage on older distros. Travis is what I use for testing complex patch series before submission, so I like it to have a useful mix of vintage OSs, not only the very latest that is largely the same as what I build on locally already.
Support for precise is going to be dropped by Travis in two months either way: https://blog.travis-ci.com/2017-08-31-trusty-as-default-status So we can keep it around for the time being if you want, but we're going to have this very same conversation again pretty soon :) -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Feb 27, 2018 at 04:53:17PM +0100, Andrea Bolognani wrote:
On Tue, 2018-02-27 at 15:41 +0000, Daniel P. Berrangé wrote:
Running syntax-check in all 5 scenarios isn't buying us anything, as the syntax-check rules don't depend on what is installed in the host. IOW, running syntax-check in 1 scenario is sufficient to get us the coverage we need.
Okay, fair enough. The change still "obfuscates" the Travis configuration though, because now you can't just look at a single script entry but you have to explode the matrix in your head and convince yourself you're covering all bases, so I'm not too happy with it.
I don't think we've got so many different scenarios here that understanding it is a real problem
It's not a massive hurdle, but it's still cognitive load that I'd rather not have to take on. See my first reply for a way of achieving the same result in a much more explicit and easy to grasp manner.
Moreover, there was a whole thing about just dropping support for precise (as Canonical already did) and making our lives easier later in the mail, but you snipped it without replying...
Opps, I'm not in favour of dropping precise, because I think it is useful to get coverage on older distros. Travis is what I use for testing complex patch series before submission, so I like it to have a useful mix of vintage OSs, not only the very latest that is largely the same as what I build on locally already.
Support for precise is going to be dropped by Travis in two months either way:
https://blog.travis-ci.com/2017-08-31-trusty-as-default-status
So we can keep it around for the time being if you want, but we're going to have this very same conversation again pretty soon :)
Actually we should not wait until then, as it leaves our stable branches with a travis config that would not work and would need fixing. So we should drop it now really Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
VPATH is not well tested by developers, so ensure travis exercises VPATH in all scenarios. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 4bdf034829..41a293451c 100644 --- a/.travis.yml +++ b/.travis.yml @@ -103,7 +103,7 @@ before_install: - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update && brew upgrade && brew install rpcgen yajl; fi before_script: - - ./autogen.sh + - mkdir build && cd build && ../autogen.sh script: # Many unit tests still fail on macOS, and there are a bunch of issues with -- 2.14.3
On Fri, 2018-02-23 at 12:00 +0000, Daniel P. Berrangé wrote:
VPATH is not well tested by developers, so ensure travis exercises VPATH in all scenarios.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.travis.yml b/.travis.yml index 4bdf034829..41a293451c 100644 --- a/.travis.yml +++ b/.travis.yml @@ -103,7 +103,7 @@ before_install: - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update && brew upgrade && brew install rpcgen yajl; fi
before_script: - - ./autogen.sh + - mkdir build && cd build && ../autogen.sh
script: # Many unit tests still fail on macOS, and there are a bunch of issues with
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 41a293451c..0328fcb8f1 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,6 +6,8 @@ matrix: include: - compiler: gcc dist: precise + env: + - CONFIGURE_ARGS=--with-init-script=upstart # Special scenario to run distcheck, so we don't waste time duplicating # work in all the other scenarios. Doesn't work on precise due to the # CVE-2012-3386 flaw being present on that Ubuntu version @@ -103,7 +105,7 @@ before_install: - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update && brew upgrade && brew install rpcgen yajl; fi before_script: - - mkdir build && cd build && ../autogen.sh + - mkdir build && cd build && ../autogen.sh $CONFIGURE_ARGS script: # Many unit tests still fail on macOS, and there are a bunch of issues with -- 2.14.3
On Fri, 2018-02-23 at 12:00 +0000, Daniel P. Berrangé wrote:
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/.travis.yml b/.travis.yml index 41a293451c..0328fcb8f1 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,6 +6,8 @@ matrix: include: - compiler: gcc dist: precise + env: + - CONFIGURE_ARGS=--with-init-script=upstart
Both precise and trusty use upstart, so there's no reason not to apply this to both, especially if we're going trusty-only as suggested earlier. Limiting it to the gcc build is rather strange as well. Even macOS doesn't seem bothered by that at all, though it's kinda nasty to install an upstart init script there. Not that it would break anything, but it just feels wrong. Perhaps we should improve our init system detection so that Ubuntu releases older than 16.04 and CentOS 6 will automatically choose upstart rather than passing this explicitly? The latter detects init system "redhat", and frankly I'm not quite sure what that's even supposed to be :) -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Feb 27, 2018 at 04:14:21PM +0100, Andrea Bolognani wrote:
On Fri, 2018-02-23 at 12:00 +0000, Daniel P. Berrangé wrote:
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/.travis.yml b/.travis.yml index 41a293451c..0328fcb8f1 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,6 +6,8 @@ matrix: include: - compiler: gcc dist: precise + env: + - CONFIGURE_ARGS=--with-init-script=upstart
Both precise and trusty use upstart, so there's no reason not to apply this to both, especially if we're going trusty-only as suggested earlier. Limiting it to the gcc build is rather strange as well.
The initscript handling code is only exercised if you run 'make install' and only the 'make distcheck' rule I added to precise will exercise 'make install'.
Even macOS doesn't seem bothered by that at all, though it's kinda nasty to install an upstart init script there. Not that it would break anything, but it just feels wrong.
We're not running 'make install' on macOS so its a no-op :-)
Perhaps we should improve our init system detection so that Ubuntu releases older than 16.04 and CentOS 6 will automatically choose upstart rather than passing this explicitly? The latter detects init system "redhat", and frankly I'm not quite sure what that's even supposed to be :)
Even though RHEL-6 supports upstart, I'm fairly sure we always deployed RHEL-6 using traditional initscripts, not the upstart scripts. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Feb 27, 2018 at 03:19:44PM +0000, Daniel P. Berrangé wrote:
On Tue, Feb 27, 2018 at 04:14:21PM +0100, Andrea Bolognani wrote:
On Fri, 2018-02-23 at 12:00 +0000, Daniel P. Berrangé wrote:
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/.travis.yml b/.travis.yml index 41a293451c..0328fcb8f1 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,6 +6,8 @@ matrix: include: - compiler: gcc dist: precise + env: + - CONFIGURE_ARGS=--with-init-script=upstart
Both precise and trusty use upstart, so there's no reason not to apply this to both, especially if we're going trusty-only as suggested earlier. Limiting it to the gcc build is rather strange as well.
The initscript handling code is only exercised if you run 'make install' and only the 'make distcheck' rule I added to precise will exercise 'make install'.
Opps, I forgot the very next patch enables make install everywhere :-)
Even macOS doesn't seem bothered by that at all, though it's kinda nasty to install an upstart init script there. Not that it would break anything, but it just feels wrong.
We're not running 'make install' on macOS so its a no-op :-)
Perhaps we should improve our init system detection so that Ubuntu releases older than 16.04 and CentOS 6 will automatically choose upstart rather than passing this explicitly? The latter detects init system "redhat", and frankly I'm not quite sure what that's even supposed to be :)
Even though RHEL-6 supports upstart, I'm fairly sure we always deployed RHEL-6 using traditional initscripts, not the upstart scripts.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 2018-02-27 at 15:19 +0000, Daniel P. Berrangé wrote:
Both precise and trusty use upstart, so there's no reason not to apply this to both, especially if we're going trusty-only as suggested earlier. Limiting it to the gcc build is rather strange as well.
The initscript handling code is only exercised if you run 'make install' and only the 'make distcheck' rule I added to precise will exercise 'make install'.
That changes with patch 4/4, where you introduce a call to 'make install' in the global script. Either way, there's no downside in having the definition in the global environment, as it makes everything tidier and easier to reason about.
Even macOS doesn't seem bothered by that at all, though it's kinda nasty to install an upstart init script there. Not that it would break anything, but it just feels wrong.
We're not running 'make install' on macOS so its a no-op :-)
Yes we are, at least as of patch 4/4.
Perhaps we should improve our init system detection so that Ubuntu releases older than 16.04 and CentOS 6 will automatically choose upstart rather than passing this explicitly? The latter detects init system "redhat", and frankly I'm not quite sure what that's even supposed to be :)
Even though RHEL-6 supports upstart, I'm fairly sure we always deployed RHEL-6 using traditional initscripts, not the upstart scripts.
So we have on Ubuntu, apparently. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Feb 27, 2018 at 04:43:18PM +0100, Andrea Bolognani wrote:
On Tue, 2018-02-27 at 15:19 +0000, Daniel P. Berrangé wrote:
Both precise and trusty use upstart, so there's no reason not to apply this to both, especially if we're going trusty-only as suggested earlier. Limiting it to the gcc build is rather strange as well.
The initscript handling code is only exercised if you run 'make install' and only the 'make distcheck' rule I added to precise will exercise 'make install'.
That changes with patch 4/4, where you introduce a call to 'make install' in the global script.
Either way, there's no downside in having the definition in the global environment, as it makes everything tidier and easier to reason about.
Even macOS doesn't seem bothered by that at all, though it's kinda nasty to install an upstart init script there. Not that it would break anything, but it just feels wrong.
We're not running 'make install' on macOS so its a no-op :-)
Yes we are, at least as of patch 4/4.
Perhaps we should improve our init system detection so that Ubuntu releases older than 16.04 and CentOS 6 will automatically choose upstart rather than passing this explicitly? The latter detects init system "redhat", and frankly I'm not quite sure what that's even supposed to be :)
Even though RHEL-6 supports upstart, I'm fairly sure we always deployed RHEL-6 using traditional initscripts, not the upstart scripts.
So we have on Ubuntu, apparently.
While upstream libvirt may be installing init scripts on Ubuntu, their own dpkg setup will use the upstart scripts :-) That is something we could usefully fix though. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 2018-02-27 at 15:46 +0000, Daniel P. Berrangé wrote:
Even though RHEL-6 supports upstart, I'm fairly sure we always deployed RHEL-6 using traditional initscripts, not the upstart scripts.
So we have on Ubuntu, apparently.
While upstream libvirt may be installing init scripts on Ubuntu, their own dpkg setup will use the upstart scripts :-) That is something we could usefully fix though.
Actually, they pass --with-init-script=none and install their own upstart script, at least in trusty. One has to wonder if there's any point in keeping around upstart integration at all these days. -- Andrea Bolognani / Red Hat / Virtualization
Running 'make install' is important to catch some VPATH problems Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 0328fcb8f1..61f0e38d40 100644 --- a/.travis.yml +++ b/.travis.yml @@ -105,12 +105,12 @@ before_install: - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update && brew upgrade && brew install rpcgen yajl; fi before_script: - - mkdir build && cd build && ../autogen.sh $CONFIGURE_ARGS + - mkdir build && cd build && ../autogen.sh $CONFIGURE_ARGS --prefix=$(pwd)/../vroot script: # Many unit tests still fail on macOS, and there are a bunch of issues with # syntax-check as well, so skip those steps on that platform for now - - make -j3 && if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check && make -j3 check; fi + - make -j3 && if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check && make -j3 check; fi && make -j3 install after_failure: - echo '============================================================================' -- 2.14.3
On Fri, 2018-02-23 at 12:01 +0000, Daniel P. Berrangé wrote:
Running 'make install' is important to catch some VPATH problems
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/.travis.yml b/.travis.yml index 0328fcb8f1..61f0e38d40 100644 --- a/.travis.yml +++ b/.travis.yml @@ -105,12 +105,12 @@ before_install: - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update && brew upgrade && brew install rpcgen yajl; fi
before_script: - - mkdir build && cd build && ../autogen.sh $CONFIGURE_ARGS + - mkdir build && cd build && ../autogen.sh $CONFIGURE_ARGS --prefix=$(pwd)/../vroot
It would be nicer if the hard-coded parameters appeared before the dynamic ones here, in case we later find out we need per-OS overrides. Also I don't like "vroot" very much, I would prefer if you used "install" instead. But that's just my preference, so feel free to disregard it.
script: # Many unit tests still fail on macOS, and there are a bunch of issues with # syntax-check as well, so skip those steps on that platform for now - - make -j3 && if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check && make -j3 check; fi + - make -j3 && if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check && make -j3 check; fi && make -j3 install
The install step should be right after building, so that the part of the command that is executed on all operating systems and the one that only applies to a subset are still visually separated. For the second hunk and with the above addressed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> The first hunk might end up being dropped due to the comments on patch 4/4; if not, and with the comments addressed, the R-b applies to it too. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Feb 27, 2018 at 04:23:01PM +0100, Andrea Bolognani wrote:
On Fri, 2018-02-23 at 12:01 +0000, Daniel P. Berrangé wrote:
Running 'make install' is important to catch some VPATH problems
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/.travis.yml b/.travis.yml index 0328fcb8f1..61f0e38d40 100644 --- a/.travis.yml +++ b/.travis.yml @@ -105,12 +105,12 @@ before_install: - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update && brew upgrade && brew install rpcgen yajl; fi
before_script: - - mkdir build && cd build && ../autogen.sh $CONFIGURE_ARGS + - mkdir build && cd build && ../autogen.sh $CONFIGURE_ARGS --prefix=$(pwd)/../vroot
It would be nicer if the hard-coded parameters appeared before the dynamic ones here, in case we later find out we need per-OS overrides.
Also I don't like "vroot" very much, I would prefer if you used "install" instead. But that's just my preference, so feel free to disregard it.
script: # Many unit tests still fail on macOS, and there are a bunch of issues with # syntax-check as well, so skip those steps on that platform for now - - make -j3 && if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check && make -j3 check; fi + - make -j3 && if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check && make -j3 check; fi && make -j3 install
The install step should be right after building, so that the part of the command that is executed on all operating systems and the one that only applies to a subset are still visually separated.
I put the 'install' bit last, because that is the least likely bit to fail, so preferrable to have 'syntax-check' / 'check' run first to fail earlier I think it would be clearer if we just put a custom script: entry under OS-X in the matrix and got rid of the conditional check. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 2018-02-27 at 15:44 +0000, Daniel P. Berrangé wrote:
script: # Many unit tests still fail on macOS, and there are a bunch of issues with # syntax-check as well, so skip those steps on that platform for now - - make -j3 && if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check && make -j3 check; fi + - make -j3 && if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check && make -j3 check; fi && make -j3 install
The install step should be right after building, so that the part of the command that is executed on all operating systems and the one that only applies to a subset are still visually separated.
I put the 'install' bit last, because that is the least likely bit to fail, so preferrable to have 'syntax-check' / 'check' run first to fail earlier
It's also the part that takes the least time, so if syntax-check or check are going to fail, running install before them won't delay the result by any significant amount of time - not to mention that the overall completion time is easily dominated by waiting in the queue for a macOS builder.
I think it would be clearer if we just put a custom script: entry under OS-X in the matrix and got rid of the conditional check.
I disagree: separating the matrix definition and the script definition results in a much more readable configuration with zero duplicated information. In fact, we used to have everything jumbled together, but we moved away from that setup. -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Daniel P. Berrangé