Skip to content
This repository was archived by the owner on Mar 28, 2018. It is now read-only.

build: remove requeriment to have a recent autoconf-archive #911

Merged
merged 2 commits into from
May 18, 2017

Conversation

gorozco1
Copy link
Contributor

@gorozco1 gorozco1 commented May 16, 2017

Asking for a recent autoconf-archive makes it extra difficult to build cc-oci-runtime in older distros: Ubuntu 16.04, Ubuntu 16.10 Fedora 24.

With this patch I am removing the requeriment for a recent autoconf-archive as well as references to AX_VALGRIND_DFLT.

Added --disable-valgrind-sgcheck to default build options

Fixes #888 and #887

@gorozco1 gorozco1 changed the title M4 version revert remove requeriment to have a really recent autoconf-archive May 16, 2017
@gorozco1 gorozco1 changed the title remove requeriment to have a really recent autoconf-archive remove requeriment to have arecent autoconf-archive May 16, 2017
@gorozco1 gorozco1 changed the title remove requeriment to have arecent autoconf-archive remove requeriment to have a recent autoconf-archive May 16, 2017
@gorozco1 gorozco1 force-pushed the m4-version-revert branch from c23a22a to a6e1cd3 Compare May 16, 2017 20:02
@gorozco1 gorozco1 changed the title remove requeriment to have a recent autoconf-archive build: remove requeriment to have a recent autoconf-archive May 16, 2017
@jcvenegas
Copy link
Contributor

It is need to call to AX_VALGRIND_DFLT before AX_VALGRIND_CHECK
https://www.gnu.org/software/autoconf-archive/ax_valgrind_check.html

])

MACRO_CHECK([AX_VALGRIND_DFLT])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This macro will check for AX_VALGRIND_CHECK AX_CODE_COVERAGE that are provided by autoconf archive.

I wonder if we can modify MACRO_CHECK, to not fail de execution but let us know if the AX_VALGRIND_CHECK and others are defined.

If they are defined the m4 files exist and we could use valgrind and code coverage, otherwise we continue with the execution without execute valgrind test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jcvenegas - I agree that that would be a nice optimisation but I don't think that needs to block this PR (since we already fail if the expected version of autoconf-archive isn't installed). Could you raise an issue to capture the idea maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree @jodh-intel , docuemented in #917, lets merge this change

@amshinde
Copy link
Contributor

@jcvenegas I think the AX_VALGRIND_DFLT is used for setting the defaults, in our case never running the sgcheck tool. We can get away with this by passing --disable-valgrind-sgcheck to configure.

@chavafg
Copy link
Contributor

chavafg commented May 16, 2017

qa-failed

Rejected with PullApprove

@chavafg
Copy link
Contributor

chavafg commented May 16, 2017

got these lines:

make[4]: Leaving directory '/home/cloud/go/src/github.com/01org/cc-oci-runtime/cc-oci-runtime-2.1.8/_build/sub'
make[3]: Leaving directory '/home/cloud/go/src/github.com/01org/cc-oci-runtime/cc-oci-runtime-2.1.8/_build/sub'
make[2]: Leaving directory '/home/cloud/go/src/github.com/01org/cc-oci-runtime/cc-oci-runtime-2.1.8/_build/sub'
Makefile:5413: recipe for target 'check-valgrind-local' failed
make[1]: *** [check-valgrind-local] Error 2
make[1]: Leaving directory '/home/cloud/go/src/github.com/01org/cc-oci-runtime/cc-oci-runtime-2.1.8/_build/sub'
Makefile:4985: recipe for target 'distcheck' failed
make: *** [distcheck] Error 1

@jcvenegas
Copy link
Contributor

@amshinde thanks for clarify , so adding adding --disable-valgrind-sgcheck should fix that issue.

This reverts commit a210cc4.

Signed-off-by: Geronimo Orozco <[email protected]>
@gorozco1 gorozco1 force-pushed the m4-version-revert branch 3 times, most recently from f94fbbd to 81abf0f Compare May 16, 2017 23:40
@chavafg
Copy link
Contributor

chavafg commented May 17, 2017

qa-failed

1 similar comment
@chavafg
Copy link
Contributor

chavafg commented May 17, 2017

qa-failed

@devimc
Copy link
Contributor

devimc commented May 17, 2017

I think @chavafg should add --disable-valgrind-sgcheck in his CI

@chavafg
Copy link
Contributor

chavafg commented May 17, 2017

The failures is happening on make distcheck, so the makefile should be updated in this line:
https://github.com/01org/cc-oci-runtime/blob/master/Makefile.am#L161

with the --disable-valgrind-sgcheck flag.

@chavafg
Copy link
Contributor

chavafg commented May 17, 2017

qa-failed

@gorozco1 gorozco1 force-pushed the m4-version-revert branch from 81abf0f to 0f92144 Compare May 17, 2017 18:14
@chavafg
Copy link
Contributor

chavafg commented May 17, 2017

qa-passed

Approved with PullApprove

@gorozco1
Copy link
Contributor Author

\o/

Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@@ -29,7 +29,8 @@ autoreconf --force --install --symlink --warnings=all
args="\
--sysconfdir=/etc \
--localstatedir=/var \
--prefix=/usr"
--prefix=/usr \
--disable-valgrind-sgcheck"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this change will automatically be applied to the OBS builds, since the files in data/obs-packaging/ call ./autogen.sh.

])

MACRO_CHECK([AX_VALGRIND_DFLT])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jcvenegas - I agree that that would be a nice optimisation but I don't think that needs to block this PR (since we already fail if the expected version of autoconf-archive isn't installed). Could you raise an issue to capture the idea maybe?

@jcvenegas
Copy link
Contributor

jcvenegas commented May 18, 2017

lgtm

Approved with PullApprove

@jcvenegas
Copy link
Contributor

@rcaballeromx could you review this change?

@jcvenegas
Copy link
Contributor

@gorozco1 could you add to the commit message that fixes #888 ( and probably ##887 ? )

AX_VALGRIND_DFLT makes it imposible to build cc-oci-runtime in older
distros. Hence we need to disable this checks in order to build
successfuly on Ubuntu 16.04, Ubuntu 16.10 and Fedora 24.

Added --disable-valgrind-sgcheck to default build options

Fixes intel#888 and intel#887

Signed-off-by: Geronimo Orozco <[email protected]>
@gorozco1 gorozco1 force-pushed the m4-version-revert branch from 0f92144 to 161aa5c Compare May 18, 2017 16:54
@amshinde
Copy link
Contributor

amshinde commented May 18, 2017

lgtm

Approved with PullApprove

gorozco1 added a commit to gorozco1/cc-oci-runtime that referenced this pull request May 18, 2017
cc-oci-runtime OBS builds needed 2 patches to build successfully on
Ubuntu 16.04, Ubuntu 16.10 and F24.

These patches already have been sent to the oficial repo on PR intel#911

Besides these patches, this commit adds improvements to the OBS
automation tooling for cc-oci-runtime, and the updated changelog sent to
the OBS builds.

Signed-off-by: Geronimo Orozco <[email protected]>
gorozco1 added a commit to gorozco1/cc-oci-runtime that referenced this pull request May 18, 2017
cc-oci-runtime OBS builds needed 2 patches to build successfully on
Ubuntu 16.04, Ubuntu 16.10 and F24.

These patches already have been sent to the oficial repo on PR intel#911

Besides these patches, this commit adds improvements to the OBS
automation tooling for cc-oci-runtime, and the updated changelog sent to
the OBS builds.

Signed-off-by: Geronimo Orozco <[email protected]>
@chavafg
Copy link
Contributor

chavafg commented May 18, 2017

qa-passed

@jcvenegas jcvenegas merged commit dff5128 into intel:master May 18, 2017
@gorozco1 gorozco1 deleted the m4-version-revert branch May 19, 2017 00:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants