set rpath for external libsemigroups #954

dimpase · 2023-09-13T21:48:19Z

set rpath for external libsemigroups, so that LD_LIBRARY_PATH is not needed if libsemigroups is installed to a non-standard location

dimpase · 2023-09-13T21:51:57Z

I've left m4/ax_check_libsemigroups.m4 largely as it was, no refactoring. I only split two very long strings into two, and
added the necessary stuff.

james-d-mitchell · 2023-09-14T08:24:26Z

Thanks @dimpase

james-d-mitchell

This seems to do exactly what is desired, happy to approve. @fingolfin could you please also have a quick look at this if you have the time?

fingolfin

Thanks! The principle seems fine to me, but I have some minor remarks on details.

fingolfin · 2023-09-14T12:32:05Z

m4/ax_check_libsemigroup.m4

@@ -53,6 +55,8 @@ AC_DEFUN([AX_CHECK_LIBSEMIGROUPS], [
  else 
        LIBSEMIGROUPS_VERSION="$(pkg-config --modversion libsemigroups)"
 	AC_MSG_NOTICE([using external libsemigroups $LIBSEMIGROUPS_VERSION])


indentation looks odd here (maybe tabs versus spaces), but that was of course already there before this PR, so not an objection to this PR.

(Indeed, this whole file mixes tabs and spaces liberally. Probably best to ignore this for this PR)

fingolfin · 2023-09-15T23:02:21Z

m4/ax_check_libsemigroup.m4

+	[AC_MSG_NOTICE([ignoring flag --with-external-libsemigroups, the Semigroups configure file
+                        was created on a system without m4 macros for pkg-config available...])])


I think this should be undone, it causes the error to change from

... checking for awk... awk configure: using included libsemigroups... configure: error: libsemigroups is required, clone or download the repo from https://github.com/libsemigroups/libsemigroups into this directory

which is sensibly wrapped in my terminal, to

checking for awk... awk configure: using included libsemigroups... configure: error: libsemigroups is required, clone or download the repo from https://github.com/libsemigroups/libsemigroups into this directory

which is rather weird.

Agreed, I think this change should be undone.

There are no two equal terminals, besides, such long lines of code aren't helping.
I can try to make sure the output is formatted better.

Have you tested the message? It was perfectly legible for me before your changes but not anymore afterwards. Was the experience different for you?

I changed it to avoid extra-long lines in the source, most of all.

fingolfin · 2023-09-15T23:02:33Z

m4/ax_check_libsemigroup.m4

+                   [AC_MSG_ERROR([libsemigroups is required, clone or download the repo from
+                                  https://github.com/libsemigroups/libsemigroups into this directory])])])


Should also be undone, IMHO

fingolfin · 2023-09-15T23:03:11Z

m4/ax_check_libsemigroup.m4

+        PKG_CHECK_VAR([LIBSEMIGROUPS_RPATH], [libsemigroups], [libdir],
+              [AC_SUBST([LIBSEMIGROUPS_RPATH],[-Wl,-rpath,${LIBSEMIGROUPS_RPATH}])])


That's the core of the change. Looks OK to me.

fingolfin · 2023-09-15T23:04:11Z

Makefile.gappkg

@@ -138,7 +138,7 @@ gen/%.$(GAP_OBJEXT): %.s Makefile
 # build rule for linking all object files together into a kernel extension
 $(KEXT_SO): $(KEXT_OBJS)
 	@mkdir -p $(@D)
-	$(QUIET_GAC)$(GAP_CXX) -o $@ $(GAP_LDFLAGS) $(GAC_LDFLAGS) $(KEXT_OBJS) $(KEXT_LDFLAGS)
+	$(QUIET_GAC)$(GAP_CXX) -o $@ $(GAP_LDFLAGS) $(GAC_LDFLAGS) $(KEXT_OBJS) $(KEXT_LDFLAGS) $(LIBSEMIGROUPS_RPATH)


I'd say this should rather be added to KEXT_LDFLAGS, that'll be better in the long run. But for now, either works shrug

james-d-mitchell · 2023-09-16T09:58:43Z

Thanks @fingolfin, @dimpase I just rebased onto main I can make Max's suggested changes if you want?

dimpase · 2023-09-16T12:39:26Z

Thanks @fingolfin, @dimpase I just rebased onto main I can make Max's suggested changes if you want?

It's up to you. I'd rather move the 2nd lines of these long error messages a bit to the left,
to reduce the unfortunate indentation.

dimpase · 2023-09-16T12:43:10Z

You can also wrap the messages in m4_normalize(), this will remove any new lines and extra spaces/tabs, I think - but keep short lines in the source.

fingolfin · 2023-09-16T14:13:52Z

which terminals do you know that do not wrap long too lines?

In any case, I am sure the message can be improved, but the current form in this PR isn't an improvement anywhere: on terminals that do wrap around it is worse, on terminals that don't it would also be worse, as they still couldn't see most of the second line.

dimpase · 2023-09-16T15:29:02Z

I think with the change I just pushed they will be again printed as one line - but split in the source.

fingolfin

Looks good to me now, thanks!

dimpase mentioned this pull request Sep 13, 2023

Possible memory leak #948

Closed

dimpase mentioned this pull request Sep 13, 2023

kernel module built on OpenBSD is broken #898

Closed

james-d-mitchell added bug-fix A label for PRs that fix a bug. build-system A label for issues or PRs related to the build system labels Sep 14, 2023

james-d-mitchell approved these changes Sep 14, 2023

View reviewed changes

fingolfin reviewed Sep 15, 2023

View reviewed changes

set rpath for external libsemigroups

eb71082

james-d-mitchell force-pushed the rpath_fix branch from bdc9239 to eb71082 Compare September 16, 2023 09:57

use m4_normalize to unsplit lines

9fba73e

fingolfin approved these changes Sep 16, 2023

View reviewed changes

james-d-mitchell merged commit 5473b4c into semigroups:main Sep 16, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set rpath for external libsemigroups #954

set rpath for external libsemigroups #954

dimpase commented Sep 13, 2023

dimpase commented Sep 13, 2023 •

edited

Loading

james-d-mitchell commented Sep 14, 2023

james-d-mitchell left a comment

fingolfin left a comment

fingolfin Sep 14, 2023

fingolfin Sep 15, 2023

fingolfin Sep 15, 2023

james-d-mitchell Sep 16, 2023

dimpase Sep 16, 2023

fingolfin Sep 16, 2023

dimpase Sep 16, 2023

fingolfin Sep 15, 2023

fingolfin Sep 15, 2023

fingolfin Sep 15, 2023

james-d-mitchell commented Sep 16, 2023

dimpase commented Sep 16, 2023

dimpase commented Sep 16, 2023

fingolfin commented Sep 16, 2023

dimpase commented Sep 16, 2023

fingolfin left a comment

		[AC_MSG_NOTICE([ignoring flag --with-external-libsemigroups, the Semigroups configure file
		was created on a system without m4 macros for pkg-config available...])])

		[AC_MSG_ERROR([libsemigroups is required, clone or download the repo from
		https://github.com/libsemigroups/libsemigroups into this directory])])])

		PKG_CHECK_VAR([LIBSEMIGROUPS_RPATH], [libsemigroups], [libdir],
		[AC_SUBST([LIBSEMIGROUPS_RPATH],[-Wl,-rpath,${LIBSEMIGROUPS_RPATH}])])

set rpath for external libsemigroups #954

set rpath for external libsemigroups #954

Conversation

dimpase commented Sep 13, 2023

dimpase commented Sep 13, 2023 • edited Loading

james-d-mitchell commented Sep 14, 2023

james-d-mitchell left a comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-d-mitchell commented Sep 16, 2023

dimpase commented Sep 16, 2023

dimpase commented Sep 16, 2023

fingolfin commented Sep 16, 2023

dimpase commented Sep 16, 2023

fingolfin left a comment

Choose a reason for hiding this comment

dimpase commented Sep 13, 2023 •

edited

Loading