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

libsemigroup upgrade to 2.7.3 #38875

Merged
merged 1 commit into from
Nov 16, 2024
Merged

libsemigroup upgrade to 2.7.3 #38875

merged 1 commit into from
Nov 16, 2024

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Oct 28, 2024

necessary for GAP 4.13.1

This was forgotten in #38804

📝 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

@dimpase
Copy link
Member Author

dimpase commented Oct 28, 2024

@orlitzky - is the patch for this in Gentoo needed here?

@dimpase dimpase mentioned this pull request Oct 28, 2024
@dimpase dimpase requested a review from culler October 28, 2024 15:54
@orlitzky
Copy link
Contributor

@orlitzky - is the patch for this in Gentoo needed here?

Not strictly but it would be nice to include it for when GCC 15 is released.

@culler
Copy link
Contributor

culler commented Oct 28, 2024

Updating the version of libsemigroups to 2.7.3 makes both the libsemigroups and gap_packages builds work for me.

Copy link
Contributor

@culler culler left a comment

Choose a reason for hiding this comment

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

Looks fine.

Copy link

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

@dimpase
Copy link
Member Author

dimpase commented Oct 28, 2024

adding the blocker tag, as it's crucial for gap_packages built (without an external to Sage correct version of libsemigroups)

@dimpase
Copy link
Member Author

dimpase commented Oct 28, 2024

@culler - could you please try this PR on a clean slate, after make distclean?
It works for me.

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).

@culler
Copy link
Contributor

culler commented Oct 28, 2024

@culler - could you please try this PR on a clean slate, after make distclean? It works for me.

I didn't do "make distclean" but I did remove all traces of libsemigroups that I could find, including the local/include/libsemigroups directory. Then I got the error that Eigen/Core did not exist. So I removed the --disable-eigen, which cause the Eigen directory to be created. Then the compiler told me to replace the angle brackets by quotes in bigraph.hpp. Then it worked.

I put my comments in the PR where --disable-eigen was being discussed.

I doubt that make distclean will work, but I can try it. If it does work I will not understand why, since it seems equivalent to what already failed for me. Also, Emmanuel was probably starting from a clean slate when he encountered the same error.

@dimpase
Copy link
Member Author

dimpase commented Oct 28, 2024

I doubt that make distclean will work, but I can try it. If it does work I will not understand why, since it seems equivalent to what already failed for me. Also, Emmanuel was probably starting from a clean slate when he encountered the same error.

Emmanual as a rule starts his upgrades without cleaning.

You (potentially) might have a copy of Eigen somewhere else, and then semigroups might get confused.
It's in fact anyone's guess what could be happening there: precompiled headers cached; ccache affects things, etc.
Few times I came across the cases where make distclean was not enough, only git clean -fdx helped; sometimes contents of ~/.sage/ affects builds.

Please try make distclean.

@culler
Copy link
Contributor

culler commented Oct 28, 2024

You (potentially) might have a copy of Eigen somewhere else,

No, not on this machine. The only Eigen's on this machine are inside the Sage_macOS application bundles that I installed in /Applications for testing. Sage's build system had better not be looking in there.

@dimpase
Copy link
Member Author

dimpase commented Oct 29, 2024

You (potentially) might have a copy of Eigen somewhere else,

No, not on this machine. The only Eigen's on this machine are inside the Sage_macOS application bundles that I installed in /Applications for testing. Sage's build system had better not be looking in there.

It's the GAP's package semigroups' build system that is doing funny things here, not Sage's build system - which cannot really have complete control over what its dependencies are doing.

@culler
Copy link
Contributor

culler commented Oct 29, 2024

It's the GAP's package semigroups' build system that is doing funny things here, not Sage's build system - which cannot really have complete control over what its dependencies are doing.

I guarantee that GAP's build system is not looking for Eigen installations in /Applications/SageMath-10.4.app. No build system would do that, no matter how many funny things it does.

@culler
Copy link
Contributor

culler commented Oct 29, 2024

After a make distclean I am able to build gap_packages. I can't explain why LIBSEMIGROUPS_EIGEN_ENABLED was not being defined when I did my previous build but is being defined after the make distclean.

Nonetheless, I think you should give my proposal due consideration. There is no reason to disable eigen when building libsemigroups. It builds with eigen enabled, if we change one line in digraph.hpp from #include <Eigen/Core> to #include "Eigen/Core", which is easy to do.

Presumably the option of using Eigen exists because using it improves the performance of libsemigroups. Why wouldn't you want that improvement as long as we have an easy way of getting it? Also, as @orlitzky said, the only reason that he disabled it was because he did not realize that libsemigroups would use its internal copy of Eigen.

@dimpase
Copy link
Member Author

dimpase commented Oct 29, 2024

I would rather add Eigen as a Sage package (on a follow up PR). Deal?

@dimpase
Copy link
Member Author

dimpase commented Oct 29, 2024

LIBSEMIGROUPS_EIGEN_ENABLED is something computed, set, and cached, by ./configure. (as it is slow to compute).

You did not clean the latter cache.

@EmmanuelCharpentier
Copy link
Contributor

The updated PR (re-fetched at Tue Oct 29 09:22:19 CET 2024) does not solve the problem. Without make dist-clean, I get the same problem as before :

[gap_packages-4.13.1] [spkg-install] g++ -std=gnu++11 -pthread -g -O2 -fPIC -MQ gen/src/cong.o -MMD -MP -MF gen/src/cong.d -std=gnu++14 -O3 -mavx -flax-vector-conversions -Igapbind14/include/ -DHPCOMBI_CONSTEXPR_FUN_ARGS -DFMT_HEADER_ONLY -DNDEBUG -g -O2 -o gen/src/cong.o -I/usr/local/sage-10/local/include/gap -I/usr/local/sage-10/local/include -DUSE_GASMAN=1 -c src/cong.cpp
[gap_packages-4.13.1] [spkg-install] In file included from /usr/local/sage-10/local/include/libsemigroups/knuth-bendix.hpp:34,
[gap_packages-4.13.1] [spkg-install]                  from /usr/local/sage-10/local/include/libsemigroups/cong.hpp:30,
[gap_packages-4.13.1] [spkg-install]                  from src/to_cpp.hpp:54,
[gap_packages-4.13.1] [spkg-install]                  from src/cong.cpp:29:
[gap_packages-4.13.1] [spkg-install] /usr/local/sage-10/local/include/libsemigroups/digraph.hpp:58:10: fatal error: Eigen/Core: No such file or directory
[gap_packages-4.13.1] [spkg-install]    58 | #include <Eigen/Core>
[gap_packages-4.13.1] [spkg-install]       |          ^~~~~~~~~~~~
[gap_packages-4.13.1] [spkg-install] compilation terminated.

HTH,

@dimpase
Copy link
Member Author

dimpase commented Oct 29, 2024

it will not work without make distclean.

A PR upgrading a package relatively often needs the latter, so I don't see a problem here.

@orlitzky
Copy link
Contributor

Presumably the option of using Eigen exists because using it improves the performance of libsemigroups. Why wouldn't you want that improvement as long as we have an easy way of getting it? Also, as @orlitzky said, the only reason that he disabled it was because he did not realize that libsemigroups would use its internal copy of Eigen.

I was just trying to explain the reasoning behind my claim that libsemigroups doesn't require pkgconf to build, I'm not responsible for the --disable-eigen.

libsemigroups is capable of using pkgconf, but our sage package shouldn't need it currently because (a) eigen is disabled and (b) the other bundled libraries that aren't disabled will use the bundled copies (i.e. no pkg-config detection).

This is related to your problem somehow but I really have no idea what happened to cause your build failure. The --disable-eigen has been there "forever" for reasons unrelated to pkg-config.

@orlitzky
Copy link
Contributor

The --disable-eigen was added in commit da558e1 as part of the last GAP 4.12.1 upgrade, but you'd have to track down the ticket to find out why. I'm sure it wasn't an unprompted fit of whimsy.

@dimpase
Copy link
Member Author

dimpase commented Oct 29, 2024

The --disable-eigen was added in commit da558e1 as part of the last GAP 4.12.1 upgrade, but you'd have to track down the ticket to find out why. I'm sure it wasn't an unprompted fit of whimsy.

AFAICS, it was added by me. The reason was that the vendored headers are not installed in a proper place this way,
and/or not supporting a multistage install.

It's a sort of a bug of GAP's semigroup package.
And libsemigroups was made a separate package to keep the complexity of gap_packages at a sane level, perhaps to make a multistage build/install possible. Sure enough, libsemigroups is a separate from semigroups project, and latter vendors the former for "easiness of installation" (cf. SageMath...).

I've just posted on sage-develop a proposal to add Eigen as a separate Sage optional package
(it's a header only C++, so easy). Please support it so that we can move on.

@EmmanuelCharpentier
Copy link
Contributor

After make distclean, ./bootstrap, ,configure $(./config.status --config, make -j8 build still triggers the same problem :

[gap_packages-4.13.1] [spkg-install] g++ -std=gnu++11 -pthread -g -O2 -fPIC -MQ gen/src/bipart.o -MMD -MP -MF gen/src/bipart.d -I./bin/include -I./bin/include/libsemigroups -std=gnu++14 -O3 -mavx -flax-vector-conversions -Igapbind14/include/ -DHPCOMBI_CONSTEXPR_FUN_ARGS -Ilibsemigroups/extern/HPCombi/include -Ilibsemigroups/extern/HPCombi/include/fallback -Ilibsemigroups/extern/fmt-8.0.1/include -Ilibsemigroups/include -DFMT_HEADER_ONLY -DNDEBUG -g -O2 -o gen/src/bipart.o -I/usr/local/sage-10/local/include/gap -I/usr/local/sage-10/local/include -DUSE_GASMAN=1 -c src/bipart.cpp
[gap_packages-4.13.1] [spkg-install] In file included from src/bipart.cpp:38:
[gap_packages-4.13.1] [spkg-install] libsemigroups/include/libsemigroups/report.hpp:42:10: fatal error: textflowcpp/TextFlow.hpp: No such file or directory
[gap_packages-4.13.1] [spkg-install]    42 | #include "textflowcpp/TextFlow.hpp"
[gap_packages-4.13.1] [spkg-install]       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
[gap_packages-4.13.1] [spkg-install] compilation terminated.

HTH,

@dimpase
Copy link
Member Author

dimpase commented Oct 29, 2024

do you have Eigen installed anywhere on the machine?

@culler
Copy link
Contributor

culler commented Oct 29, 2024

LIBSEMIGROUPS_EIGEN_ENABLED is something computed, set, and cached, by ./configure. (as it is slow to compute).

You did not clean the latter cache.

Maybe, but I don't think this explains everything. Emmanual saw the same error (Eigen/Core does not exist) and he would never have run a build without the -disable-eigen flag being set.

@dimpase
Copy link
Member Author

dimpase commented Oct 29, 2024

We are still to hear from Emmanuel whether he ha s another instance of Eigen installed on the machine

@EmmanuelCharpentier
Copy link
Contributor

EmmanuelCharpentier commented Oct 29, 2024

We are still to hear from Emmanuel whether he ha s another instance of Eigen installed on the machine

Sorry, I was out for a short beer... ;-)

Well, yeah :

  • a subdirectory of the cmdstan Bayesian analysis system is named Eigen;
  • an R package named RcppEigen has a ubdirectory namen Eigen ;
  • ditto for the R package nimble ;
  • /usr/local/sage-10/local/include/libsemigroups/Eigen : this is my $SAGE_ROOT ; origin unknown (I did make distclean...)
  • /usr/local/sage-10/local/var/tmp/sage/build/gap_packages-4.13.1/src/pkg/semigroups/libsemigroups/extern/eigen-3.3.9/Eigen: ditto...
  • plus some copies outside the path or my user files (e. g. a backup from an installation on a previous machine...).

EDIT : FWIW :

charpent@SAP5342949:/usr/local/sage-10$ locate Eigen | wc
   2443    2443  231298

HTH,

@dimpase
Copy link
Member Author

dimpase commented Oct 29, 2024

I certainly don't have $SAGEROOT/local/include/libsemigroups/Eigen/ on a macOS system I tried this PR.
Trying on Linux now.

On Linux I don't get any of these either. Thus it must be a leftover from a previous install.
(because make distclean wipes out the whole local/.)

@dimpase
Copy link
Member Author

dimpase commented Oct 29, 2024

We can add a spkg-legacy-uninstall doing rm -rf $SAGEROOT/local/include/libsemigroups/ - other than that, I don't know how else it can be helped

@enriqueartal
Copy link
Contributor

I have just seen the positive review. I can confirm that installations that failed with the previous version work now (with beta9 merged).

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 5, 2024
    
necessary for GAP 4.13.1

This was forgotten in sagemath#38804


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ 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

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38875
Reported by: Dima Pasechnik
Reviewer(s): Marc Culler
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 6, 2024
    
necessary for GAP 4.13.1

This was forgotten in sagemath#38804


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ 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

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38875
Reported by: Dima Pasechnik
Reviewer(s): Marc Culler
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 7, 2024
    
necessary for GAP 4.13.1

This was forgotten in sagemath#38804


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ 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

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38875
Reported by: Dima Pasechnik
Reviewer(s): Marc Culler
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 8, 2024
    
necessary for GAP 4.13.1

This was forgotten in sagemath#38804


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ 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

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38875
Reported by: Dima Pasechnik
Reviewer(s): Marc Culler
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 9, 2024
    
necessary for GAP 4.13.1

This was forgotten in sagemath#38804


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ 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

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38875
Reported by: Dima Pasechnik
Reviewer(s): Marc Culler
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 13, 2024
    
necessary for GAP 4.13.1

This was forgotten in sagemath#38804


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ 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

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38875
Reported by: Dima Pasechnik
Reviewer(s): Marc Culler
@vbraun vbraun merged commit 5a8cfd0 into sagemath:develop Nov 16, 2024
26 of 55 checks passed
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.

6 participants