Fix GNU readline detection on OpenBSD, and make the configure test for it more robust #3428

dimpase · 2019-04-29T08:09:17Z

Description

this fixes #3426.

Text for release notes

Better handling of incompatible with GAP readline libraries.
Enable building of GAP on OpenBSD (with ereadline fixed as in
https://marc.info/?l=openbsd-ports&m=155646111509306&w=2)

Further details

Do not forget to run ./autogen + ./configure before testing this.

configure.ac

codecov · 2019-04-30T06:14:07Z

Codecov Report

Merging #3428 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3428      +/-   ##
==========================================
+ Coverage   85.17%   85.19%   +0.01%     
==========================================
  Files         699      697       -2     
  Lines      346081   343975    -2106     
==========================================
- Hits       294784   293037    -1747     
+ Misses      51297    50938     -359

Impacted Files	Coverage Δ
src/modules.h	`33.33% <0%> (-66.67%)`	⬇️
src/cyclotom.h	`50% <0%> (-50%)`	⬇️
src/records.h	`50% <0%> (-50%)`	⬇️
src/gapstate.h	`42.85% <0%> (-42.86%)`	⬇️
src/vec8bit.h	`51.78% <0%> (-42.86%)`	⬇️
src/macfloat.h	`57.14% <0%> (-42.86%)`	⬇️
src/code.h	`57.14% <0%> (-40.16%)`	⬇️
src/gvars.h	`66.66% <0%> (-33.34%)`	⬇️
src/hookintrprtr.h	`66.66% <0%> (-33.34%)`	⬇️
src/fibhash.h	`66.66% <0%> (-33.34%)`	⬇️
... and 104 more

coveralls · 2019-04-30T07:32:37Z

Coverage remained the same at 84.675% when pulling 9b1c61f on dimpase:master into 10a05b2 on gap-system:master.

fingolfin · 2019-05-03T13:50:46Z

I adjusted the title of this PR to be more suitable for release notes; please re-edit if you disagree; you may also want to change the commit message of your first commit, as it still reference OSX; and squash the second commit into the first.

DominikBernhardt · 2019-08-17T12:49:22Z

@dimpase Ping.

dimpase · 2019-11-20T11:54:19Z

yep, I should get back to this.

dimpase · 2019-11-25T18:29:03Z

Ready for review

fingolfin · 2019-11-26T10:40:29Z

@dimpase thanks for the update, I'll take a look. But the commit message still is odd; it says "also, do not wrongly accuse OSX of wrongdoing" but the only other reference to OSX is in the first line of the commit message? So maybe just remove both mentions -- or does OSX have any relevance for this PR after all?

dimpase · 2019-11-26T10:51:09Z

commit message still is odd

It probably referred to some already removed comment in configure.ac. I'll edit the commit message, no problem. Needless to say, this change works on OSX if --with-readline=/foo/bar is provided, and readline installed in /foo/bar.

It also should be mentioned that this fixes a bug: testing for rl_ding did not identify the needed readline version, so now the test for a newer rl_... finction.

dimpase · 2019-11-26T10:58:30Z

imroved commit message, no other changes

fingolfin

OK, this looks pretty good, but autoconf's horrible syntax with too many braces, brackets, parens and no keywords makes it rather hard to see what is nested where, so I want to check the code out locally now. In the meantime, a few comments/questions. And thanks for working on this!

configure.ac

fingolfin

Looks good now up to some very minor nitpicks, thanks!

dimpase · 2019-11-27T10:57:54Z

@kaashif - perhaps this is now ready for making an OpenBSD GAP package.

fingolfin

Thanks, that LGTM. Would you mind squashing the second commit?

rl_ding test does not correctly identify the needed readline version, so this is changed to rl_bind_keyseq. OpenBSD installs modern readline as libereadline in /usr/local, and its headers in /usr/local/include/ereadline/ Thus we provide custom -I in CFLAGS and custom -L in LDFLAGS even if we use --with-readline=yes without an explicit path in case of libereadline detected. Address PR 3428 reviewer comments

dimpase · 2019-11-27T12:53:41Z

squashed. Thanks for taking care of this.

fingolfin · 2019-11-27T14:30:47Z

@ChrisJefferson @alex-konovalov I'd like to backport to this to stable-4.11, OK?

ChrisJefferson · 2019-11-27T15:19:48Z

That's fine with me

kaashif · 2019-11-27T20:20:45Z

@dimpase Maybe. I'll make another pass on the weekend at making a package and submitting it.

fingolfin · 2019-11-27T20:28:28Z

For packaging, I recommend talking with/to us -- we can often provide hints how to overcome certain problems. Even if you get stuck and decide to abandon the effort, we are grateful for any constructive feedback, to help improve things (In the end, lack of time/resources/contributors sadly is always a limiting factor)

dimpase · 2019-11-27T20:32:40Z

@kaashif so to build on OpenBSD, one needs to pkg_add gmp and ereadline and run

./autogen.sh
CC=clang CXX=clang++ ./configure --with-readline=/usr/local --with-gmp=/usr/local
gmake

installation should be tested with LC_CTYPE=en_US.UTF-8 - otherwise there are weird UTF-8 problems - see #2194

fingolfin · 2019-11-28T08:26:58Z

Backported to stable-4.11 as f67d98a

dimpase requested a review from fingolfin April 29, 2019 08:47

fingolfin requested changes Apr 29, 2019

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

configure.ac Show resolved Hide resolved

configure.ac Outdated Show resolved Hide resolved

configure.ac Outdated Show resolved Hide resolved

fingolfin added the topic: build system label Apr 29, 2019

fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels May 3, 2019

fingolfin changed the title ~~better readline treatment on OpenBSD and OSX~~ better readline treatment on OpenBSD May 3, 2019

fingolfin changed the title ~~better readline treatment on OpenBSD~~ Link against correct readline implementation on OpenBSD May 3, 2019

dimpase force-pushed the master branch from 2573399 to ab76d62 Compare November 25, 2019 18:01

dimpase force-pushed the master branch from ab76d62 to dea4ab8 Compare November 26, 2019 10:57

fingolfin reviewed Nov 26, 2019

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

configure.ac Show resolved Hide resolved

configure.ac Outdated Show resolved Hide resolved

fingolfin reviewed Nov 26, 2019

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

fingolfin changed the title ~~Link against correct readline implementation on OpenBSD~~ Fix GNU readline support on OpenBSD, and make the configure test for it more robust Nov 26, 2019

fingolfin approved these changes Nov 26, 2019

View reviewed changes

fingolfin approved these changes Nov 27, 2019

View reviewed changes

dimpase force-pushed the master branch from 4df77b2 to 9b1c61f Compare November 27, 2019 11:56

fingolfin merged commit 9378325 into gap-system:master Nov 27, 2019

fingolfin added the backport-to-4.11 label Nov 27, 2019

fingolfin added backport-to-4.11-DONE and removed backport-to-4.11 labels Nov 28, 2019

fingolfin changed the title ~~Fix GNU readline support on OpenBSD, and make the configure test for it more robust~~ Fix GNU readline detection on OpenBSD, and make the configure test for it more robust Nov 28, 2019

fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Dec 5, 2019

olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020

olexandr-konovalov mentioned this pull request Feb 19, 2021

Simplify the creation of release notes #4257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GNU readline detection on OpenBSD, and make the configure test for it more robust #3428

Fix GNU readline detection on OpenBSD, and make the configure test for it more robust #3428

dimpase commented Apr 29, 2019 •

edited by wilfwilson

Loading

codecov bot commented Apr 30, 2019

coveralls commented Apr 30, 2019 •

edited

Loading

fingolfin commented May 3, 2019

DominikBernhardt commented Aug 17, 2019

dimpase commented Nov 20, 2019

dimpase commented Nov 25, 2019

fingolfin commented Nov 26, 2019

dimpase commented Nov 26, 2019

dimpase commented Nov 26, 2019

fingolfin left a comment

fingolfin left a comment

dimpase commented Nov 27, 2019

fingolfin left a comment

dimpase commented Nov 27, 2019

fingolfin commented Nov 27, 2019

ChrisJefferson commented Nov 27, 2019

kaashif commented Nov 27, 2019 •

edited

Loading

fingolfin commented Nov 27, 2019

dimpase commented Nov 27, 2019

fingolfin commented Nov 28, 2019

Fix GNU readline detection on OpenBSD, and make the configure test for it more robust #3428

Fix GNU readline detection on OpenBSD, and make the configure test for it more robust #3428

Conversation

dimpase commented Apr 29, 2019 • edited by wilfwilson Loading

Description

Text for release notes

Further details

codecov bot commented Apr 30, 2019

Codecov Report

coveralls commented Apr 30, 2019 • edited Loading

fingolfin commented May 3, 2019

DominikBernhardt commented Aug 17, 2019

dimpase commented Nov 20, 2019

dimpase commented Nov 25, 2019

fingolfin commented Nov 26, 2019

dimpase commented Nov 26, 2019

dimpase commented Nov 26, 2019

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

dimpase commented Nov 27, 2019

fingolfin left a comment

Choose a reason for hiding this comment

dimpase commented Nov 27, 2019

fingolfin commented Nov 27, 2019

ChrisJefferson commented Nov 27, 2019

kaashif commented Nov 27, 2019 • edited Loading

fingolfin commented Nov 27, 2019

dimpase commented Nov 27, 2019

fingolfin commented Nov 28, 2019

dimpase commented Apr 29, 2019 •

edited by wilfwilson

Loading

coveralls commented Apr 30, 2019 •

edited

Loading

kaashif commented Nov 27, 2019 •

edited

Loading