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

install: Extract autoconf-archive install to separate script #876

Merged
merged 1 commit into from
Jul 3, 2017

Conversation

grahamwhaley
Copy link

Under some circumstances it is needed to 'hand install' the m4
scripts. This was done in the rhel install script, but sometimes
it is really useful to be able to do that by hand, so extract the
relevant commands into their own script (to allow hand running),
and get the rhel script to invoke that new script at the appropriate
time.

Fixes: #875

Signed-off-by: Graham Whaley [email protected]

@grahamwhaley
Copy link
Author

Marked this as WIP, as I am not set up to re-test the modified rhel install scripts. @devimc or @chavafg - are either of you in a position to try this out for me?

#

# autoconf-archive url
autoconf_archive_url="http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=blob_plain;f=m4"
Copy link
Contributor

Choose a reason for hiding this comment

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

I recently added autoconf_archive to Clear Linux, what do you think to chanage that url with something like this
https://ftp.gnu.org/gnu/autoconf-archive/autoconf-archive-2017.03.21.tar.xz

Copy link
Author

Choose a reason for hiding this comment

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

I think I like the versioned link better than the gitweb one, yes.
What do you think - we can just pull the tar file and extract just the files we need into the m4 directory with a single piped tar command maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this decision effectively summarises to:

  • do we want to remain on the development tip of autoconf-archive (gitweb)? or,
  • do we want stability at the expense of using old code (ftp)?

I'd leaning slightly towards the former since otherwise we have something else to maintain (if we go with the latter we should add the package version to versions.txt). If the GNU tip does break, we can raise a PR to revert to the last good commit. That said, I can't imagine that the two m4 files we're using are going to change significantly enough to warrant tracking the top of development. However, by remaining current, we'll ensure RHEL/CentOS are more in line with the other distros (which presumably will update the autoconf-archive package as new versions are released upstream.

Copy link
Author

Choose a reason for hiding this comment

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

Nice breakdown @jodh-intel With that info, I think I agree that longer term the development tip is safe enough (these are very low churn files)...
@jcvenegas , what say you?

Copy link
Contributor

Choose a reason for hiding this comment

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

@grahamwhaley @jodh-intel , not problem here, I was curious to be a consistent with the versions.txt, it already includes a version for "autoconf_archive_version", and in case of that happen, we potentially could know about it and fix it quickly

@jcvenegas
Copy link
Contributor

Looks good looks very helpful ! I think it could be nice to have some documentation about it somewhere or add an option to configure.ac to do something like:

./configure --with-download-autoconf-archive

@grahamwhaley
Copy link
Author

@jcvenegas I mocked up some docs locally that lived as an indented paragraph in here: https://github.com/01org/cc-oci-runtime/blame/master/README.rst#L209 but, it feels odd, and I'm not sure that is the correct or best place. @jodh-intel any thoughts on this?
We have issues that contain the trail with error and workaround, so I'm not unhappy to proceed without an explicit document being present.

@grahamwhaley
Copy link
Author

@jcvenegas @devimc @jodh-intel For the autoconf addition... two things:

  • I'll be interested if somebody has a view here - I have no objection, just want to collect a collective view
  • I don't have the hands on auto-magic skills to easily locate where and how to implement this - if somebody wants to give some sample code/patch then please feel free - I could either then merge that into my PR, or we can merge this PR and you can build on top of it.

@jcvenegas
Copy link
Contributor

@grahamwhaley sure lets make the autoconf thing not a blocker, we can added it latter if needed

@grahamwhaley
Copy link
Author

Just to nudge this forwards, here is the doc snippet (in a temporary branch/commit) I tried out.
grahamwhaley@92a9ef7
I'm not too happy with that myself, so;

  • if somebody can come up with a better place or way to doc it, please suggest
  • or, given the details of the problem and workaround are in the github history where folks might search for such things, we could just go ahead and commit as is
  • or, if this patch set is being outdated by the pending autoconf archive versions PR build: remove requeriment to have a recent autoconf-archive #911 , then once that is sorted out and merged let me know and we can close this one down.

@grahamwhaley
Copy link
Author

@gorozco1 @jodh-intel with #911 , is this PR now redundant - shall I shut it down?

@jodh-intel
Copy link
Contributor

I think it is still useful for RHEL and CentOS which don't provide an autoconf-archive package.

@jodh-intel
Copy link
Contributor

Due to the problems we're experiencing with the http://savannah.gnu.org/ site (see #946), I think this script is definitely needed, but should download the whole archive and extract just the files we need. A mirror like kernel.org (http://mirrors.kernel.org/gnu/autoconf-archive/) would seem like a good bet for reliability but we'll have to add "some" version of autoconf-archive to versions.txt.

@gorozco1
Copy link
Contributor

gorozco1 commented Jun 5, 2017

@grahamwhaley I think this script is still needed for RHEL/CentOS

@jodh-intel jodh-intel mentioned this pull request Jun 5, 2017
@jodh-intel
Copy link
Contributor

Update: savannah is back so we have some thinking time now ;)

@grahamwhaley
Copy link
Author

This has been hanging here for some time - it is 'self documenting' in the Issue, and mostly we have moved on past this issue, but the script is still useful. Drop the DNM tag, and let's merge it.

@jodh-intel
Copy link
Contributor

jodh-intel commented Jun 29, 2017

lgtm

Approved with PullApprove

@chavafg
Copy link
Contributor

chavafg commented Jun 29, 2017

qa-failed

Rejected with PullApprove

@jcvenegas
Copy link
Contributor

@chavafg any information why tests failed ?

@chavafg
Copy link
Contributor

chavafg commented Jun 29, 2017

Seems that PR needs a rebase, so it runs the swarm tests with the nginx image that we currently support.

@jcvenegas
Copy link
Contributor

@grahamwhaley could you rebase the PR ?

Under some circumstances it is needed to 'hand install' the m4
scripts. This was done in the rhel install script, but sometimes
it is really useful to be able to do that by hand, so extract the
relevant commands into their own script (to allow hand running),
and get the rhel script to invoke that new script at the appropriate
time.

Fixes: intel#875

Signed-off-by: Graham Whaley <[email protected]>
@grahamwhaley grahamwhaley force-pushed the 20170504_curl_auto_macros branch from 78560f5 to 82cfb1f Compare June 30, 2017 10:29
@grahamwhaley
Copy link
Author

Rebase done and pushed. I am not set up to easily verify this patch at present, but a visual check shows the changes appear identical after the rebase. If somebody feels they are set up and would like to do a test run of the rhel installer, please do.

@chavafg
Copy link
Contributor

chavafg commented Jun 30, 2017

qa-failed

@chavafg
Copy link
Contributor

chavafg commented Jun 30, 2017

this is a qa-passed, but for some reason old logs didn't get cleared and they make this failed. Will look into the ci issue.

@chavafg
Copy link
Contributor

chavafg commented Jun 30, 2017

qa-passed

Approved with PullApprove

@devimc
Copy link
Contributor

devimc commented Jul 3, 2017

lgtm

Approved with PullApprove

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants