Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkgconf to dependencies of libsemigroups #38870

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Oct 28, 2024

without it, libsemigroups will be useless for GAP's semigroups package

see sage-release for a discussion

📝 Checklist

  • [x ] The title is concise and informative.
  • [x ] The description explains in detail what this PR is about.
  • [x ] I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

without it, libsemigroups will be useless for GAP's semigroups package
Copy link

Documentation preview for this PR (built with commit 9785adf; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@orlitzky
Copy link
Contributor

I think pkgconf should be a (build only, after the pipe) dependency of gap_packages instead, right? The GAP semigroups package uses it to detect libsemigroups: https://github.com/semigroups/Semigroups/blob/main/m4/ax_check_libsemigroup.m4

libsemigroups, on the other hand, only uses pkgconf to detect things that sage disables anyway:

https://github.com/search?q=repo%3Alibsemigroups%2Flibsemigroups%20PKG_CHECK_MODULES&type=code

(I guess we could make this more explicit with --disable-eigen --disable-hpcombi --disable-fmt)

@culler
Copy link
Contributor

culler commented Oct 28, 2024

I don't think Eigen was disabled in the previous version of the libsemigroups package. When I fixed the issue with TextFlow.hpp I got another error in the include statement in the following block of code from digraph.hpp:

#ifdef LIBSEMIGROUPS_EIGEN_ENABLED
#include <Eigen/Core>
#endif

I think that means that LIBSEMIGROUPS_EIGEN_ENABLED was defined in the previous version.

@orlitzky
Copy link
Contributor

The spkg-install for libsemigroups does,

cd src
sdh_configure --disable-eigen
sdh_make
sdh_make_install

so unless that's buggy somehow, it should be disabled (we don't have eigen in the sage distro).

@culler
Copy link
Contributor

culler commented Oct 28, 2024

I was able to successfully build libsemigroups and gap_packages after making these two changes.

  1. Remove the --disable-eigen option, so that sage/build/pkgs/libsemigroups/spkg-install.in looks like:
cd src
sdh_configure
sdh_make
sdh_make_install
  1. Add a post-install script sage/build/pkgs/libsemigroups/spkg-postinst.in containing:
sed -i '' 's|<Eigen/Core>|"Eigen/Core"|' ${SAGE_LOCAL}/include/libsemigroups/digraph.hpp

It should be possible to make the include statement in digraph.hpp work correctly by adding a -I option somewhere, but I don't know where to do that and this worked for me.

I also don't know what it means when @orlitzky says "we don't have eigen in the sage distro".

@orlitzky
Copy link
Contributor

I also don't know what it means when @orlitzky says "we don't have eigen in the sage distro".

I meant that we don't have build/pkgs/eigen, which I took for even more evidence that --disable-eigen had to disable it. But now I see that it's bundled with libsemigroups, so I guess it's possible that --disable-eigen was not working somehow and that it used the bundled version.

@dimpase
Copy link
Member Author

dimpase commented Oct 28, 2024

Sorry, guys, this PR has little to do with #38875 - could you please review #38875 there, and not here.

This PR fixes a potential bug which might occur if one runs make gap_packages right after ./configure.

@dimpase
Copy link
Member Author

dimpase commented Oct 28, 2024

And could you please review this PR?

@dimpase
Copy link
Member Author

dimpase commented Oct 28, 2024

I also don't know what it means when @orlitzky says "we don't have eigen in the sage distro".

I meant that we don't have build/pkgs/eigen, which I took for even more evidence that --disable-eigen had to disable it. But now I see that it's bundled with libsemigroups, so I guess it's possible that --disable-eigen was not working somehow and that it used the bundled version.

For me, everything builds fine after make distclean (even on macOS without any Eigen or libsemigroup installed anywhere, I think). I'll repeat this comment on #38875 and would like to continue there.

@orlitzky
Copy link
Contributor

And could you please review this PR?

I was. I think this commit changes the wrong package:

  • libsemigroups doesn't need pkgconf
  • gap_packages does

@dimpase
Copy link
Member Author

dimpase commented Oct 28, 2024

And could you please review this PR?

I was. I think this commit changes the wrong package:

* libsemigroups doesn't need pkgconf

* gap_packages does

libsemigroups could not be used by its Sage consumer(s) without pkg-config. And (probably) it might fail tests.
There is also a danger that libsemigroups won't even install it .pc file if pkg-config is not available.

@orlitzky
Copy link
Contributor

libsemigroups could not be used by its Sage consumer(s) without pkg-config.

Only the consumers that rely on pkg-config for detection need pkg-config. Others would be fine without it. The packages that need pkg-config should depend on pkg-config...

And (probably) it might fail tests.

You can quickpkg up your dev-util/pkgconf, emerge -C it, and try :)

There is also a danger that libsemigroups won't even install it .pc file if pkg-config is not available.

This is handled by automake which doesn't know anything about pkg-config: https://github.com/libsemigroups/libsemigroups/blob/main/Makefile.am#L39

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

Successfully merging this pull request may close these issues.

3 participants