`build/pkgs/tdlib`: Update to 0.9.3 by mkoeppe · Pull Request #38163 · sagemath/sage

mkoeppe · 2024-06-07T00:04:27Z

FIxes try to upgrade tdlib #30813

📝 Checklist

The title is concise and informative.
The description explains in detail what this PR is about.
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

Depends on CI: Fix handling of deletions/renames; Build&Test: Uninstall/build changed packages; Linux Incremental: Update platforms #38144 (merged here for testing)

…el condition

…-linux-incremental.yml)

…container if nothing to be tested

…of the default platform (ubuntu-jammy)

…dora-38 by latest

…t from build.yml, use ptest-nodoc to avoid duplicate doc-html build

… /new at the end

…e new .git file at the end

…158b6)

…tlab.archlinux.org/archlinux/packaging/packages/sagemath/-/blob/main/sagemath-tdlib-0.9.patch\?ref_type\=heads

mkoeppe · 2024-06-07T00:42:24Z

with boost_cropped from SPKG:

[tdlib-0.9.1] [spkg-install] checking for boost/tuple/tuple.hpp... yes
[tdlib-0.9.1] [spkg-install] checking for main in -lboost_system... no
[tdlib-0.9.1] [spkg-install] configure: error: Unable to find Boost System library

https://gitlab.com/freetdi/treedec/-/blob/develop/configure.ac?ref_type=heads#L136

mkoeppe · 2024-06-07T00:51:27Z

@felix-salfelder with boost from homebrew, boost_system is found but:

configure:16743: checking for main in -lboost_thread
configure:16762: g++ -std=gnu++11 -o conftest -g -O2 -std=c++11  -L/Users/mkoeppe/s/sage/sage-rebasing/local/lib -L/Users/mkoeppe/s/sage/sage-rebasing/local/lib  -Wl,-ld_classic conftest.cpp -lboost_thread  -lboost_system  >&5
ld: library not found for -lboost_thread
clang: error: linker command failed with exit code 1 (use -v to see invocation)

$ ls /usr/local/opt/boost/lib/libboost_thread
/usr/local/opt/boost/lib/libboost_thread-mt.a     
/usr/local/opt/boost/lib/libboost_thread-mt.dylib

felix-salfelder · 2024-06-07T01:26:18Z

I am not sure what -mt is, and why the plain one is missing. Are they interchangeable?

The cython bindings should not require boost_thread. Consider removing the test from configure.

mkoeppe · 2024-06-07T01:29:59Z

Do you think we can also get rid of boost_system?

mkoeppe · 2024-06-07T01:50:30Z

Wait, is this whole thing header-only?

felix-salfelder · 2024-06-07T01:51:01Z

On Thu, Jun 06, 2024 at 06:30:23PM -0700, Matthias Köppe wrote: Do you think we can also get rid of `boost_system`?

Yes. Probably configure should succeed regardless, and tdecomp compilation should be made conditional on the check. tdlib should build/link/work either way, please try. I'll look closer and do the changes when I find the time.

mkoeppe · 2024-06-07T01:56:31Z

Perhaps disable the check for boost_system, boost_thread on --without-gala, which turns off TDECOMP?

mkoeppe · 2024-06-07T02:13:48Z

From 63a7306e5c8a5904b79497f04139fd1d9c96bf6a Mon Sep 17 00:00:00 2001
From: Matthias Koeppe <[email protected]>
Date: Thu, 6 Jun 2024 19:11:18 -0700
Subject: [PATCH] configure.ac: Only check for boost_system, boost_thread when
 configured --with-gala

---
 configure.ac | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/configure.ac b/configure.ac
index ea36ffb..a643c50 100644
--- a/configure.ac
+++ b/configure.ac
@@ -133,23 +133,24 @@ CXXFLAGS="-std=c++11 $CXXFLAGS"
 AC_CHECK_HEADERS([boost/tuple/tuple.hpp], [],
                  AC_MSG_ERROR([could not find a boost header])
 )
-AC_CHECK_LIB([boost_system], [main], ,
-             [AC_MSG_ERROR([Unable to find Boost System library])])
-AC_CHECK_LIB([boost_thread], [main], ,
-             [AC_MSG_ERROR([Unable to find Boost Thread library])])
 
 have_gala=no
 
 AC_ARG_WITH([gala], AS_HELP_STRING([--without-gala], [Disable gala]))
 
-AS_IF([test "x$with_gala" != xno], [
-	AC_CHECK_HEADERS([gala/graph.h],
-	                 [ have_gala=yes ],
-	                   AC_MSG_NOTICE([consider installing gala])
-	)
-   AS_IF([test "x$with_gala" = xyes -a x$have_gala = xno], [
-									 AC_MSG_ERROR([gala not found, but set])
-		 ])
+AS_IF([test "x$with_gala" != xno], [dnl
+    AC_CHECK_HEADERS([gala/graph.h],
+                     [have_gala=yes
+                      AC_CHECK_LIB([boost_system], [main], ,
+                                   [AC_MSG_ERROR([Unable to find Boost System library, required when building --with-gala])])
+                      AC_CHECK_LIB([boost_thread], [main], ,
+                                   [AC_MSG_ERROR([Unable to find Boost Thread library, required when building --with-gala])])
+                     ], [dnl
+                      AC_MSG_NOTICE([consider installing gala])
+                     ])
+    AS_IF([test "x$with_gala" = xyes -a x$have_gala = xno], [dnl
+      AC_MSG_ERROR([gala not found, but set])
+    ])
 ])
 
 AM_CONDITIONAL([USE_GALA], [test $have_gala = yes])
-- 
2.42.0

@felix-salfelder Or I can send a PR/MR if that's better

felix-salfelder · 2024-06-07T02:44:12Z

On Thu, Jun 06, 2024 at 07:14:10PM -0700, Matthias Köppe wrote: >From 63a7306e5c8a5904b79497f04139fd1d9c96bf6a Mon Sep 17 00:00:00 2001 From: Matthias Koeppe ***@***.***> Date: Thu, 6 Jun 2024 19:11:18 -0700 Subject: [PATCH] configure.ac: Only check for boost_system, boost_thread when configured --with-gala

The checks need to stay independent. I fixed the logic a little -- "develop" might just work for you as well. I don't like the way LIBS is set globally by AC_CHECK_LIBS. Another time...

mkoeppe · 2024-06-07T02:48:14Z

Thanks, current develop looks fine.
Would you be able to tag a release for us?

mkoeppe · 2024-06-07T03:25:38Z

With the current branch here on the PR, it appears to work well on fedora-40-standard (where system boost is sufficiently complete) - https://github.com/sagemath/sage/actions/runs/9409800932/job/25920443039?pr=38163

dcoudert

LGTM.

mkoeppe · 2024-06-09T00:23:39Z

Thanks!

sagemathgh-38163: `build/pkgs/tdlib`: Update to 0.9.3       - FIxes sagemath#30813 - Fixes sagemath#38159 ### 📝 Checklist  - [x] The title is concise and informative. - [ ] 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    - Depends on sagemath#38144 (merged here for testing) URL: sagemath#38163 Reported by: Matthias Köppe Reviewer(s): David Coudert

dcoudert · 2024-06-10T15:07:25Z

@dimpase is right that we are missing a text showing that the issue is fixed (#38159 (comment)).

mkoeppe · 2024-06-10T16:31:28Z

Update PRs are not in charge of adding such tests

mkoeppe · 2024-06-10T16:31:44Z

Just open a PR for adding this.

mkoeppe · 2024-06-10T21:59:07Z

I've removed the claim regarding #38159 from the PR description.
I'll leave investigating this to interested third parties.

saraedum · 2024-06-11T10:31:50Z

It appears that there is disagreement over whether this "needs work" or not. I understand that @dimpase has outlined his points in the comments in #38192 and #38190. It appears to me that this is a "disputed" PR now.

So, let me record a -1 for @dimpase. (Please reach out to me if that's not what you had in mind.)

sagemathgh-38163: `build/pkgs/tdlib`: Update to 0.9.3       - FIxes sagemath#30813 ### 📝 Checklist  - [x] The title is concise and informative. - [ ] 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    - Depends on sagemath#38144 (merged here for testing) URL: sagemath#38163 Reported by: Matthias Köppe Reviewer(s): David Coudert

sagemathgh-38214: test that sagemath#38159 is fixed This will fix sagemath#38159 Add a missing test ### 📝 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. - [ x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies   sagemath#38163 URL: sagemath#38214 Reported by: Dima Pasechnik Reviewer(s): David Coudert

sagemathgh-38163: `build/pkgs/tdlib`: Update to 0.9.3       - FIxes sagemath#30813 ### 📝 Checklist  - [x] The title is concise and informative. - [ ] 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    - Depends on sagemath#38144 (merged here for testing) URL: sagemath#38163 Reported by: Matthias Köppe Reviewer(s): David Coudert

sagemathgh-38214: test that sagemath#38159 is fixed This will fix sagemath#38159 Add a missing test ### 📝 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. - [ x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies   sagemath#38163 URL: sagemath#38214 Reported by: Dima Pasechnik Reviewer(s): David Coudert

Matthias Koeppe added 18 commits June 2, 2024 22:50

.github/workflows/ci-linux-incremental.yml (sitepackages): Remove lab…

421b552

…el condition

.github/workflows/build.yml: Determine build_targets (adapted from ci…

e90d737

…-linux-incremental.yml)

.github/workflows/build.yml: Combine calls of tj-actions/changed-files

dae0ff1

.github/workflows/build.yml (test-new): Do not push and do not start …

62ade48

…container if nothing to be tested

.github/workflows/ci-linux-incremental.yml: Remove duplicate testing …

9d3a3f2

…of the default platform (ubuntu-jammy)

.github/workflows/ci-linux-incremental.yml: Replace ubuntu-mantic, fe…

4db3313

…dora-38 by latest

.github/workflows/ci-linux-incremental.yml: Update build_target scrip…

3586090

…t from build.yml, use ptest-nodoc to avoid duplicate doc-html build

.ci/retrofit-worktree.sh: Commit /new

1df4f5a

.ci/write-dockerfile.sh: Remove old /new at the beginning, remove new…

8d0c849

… /new at the end

.ci/write-dockerfile.sh: Remove duplicated ADD

d068c8d

.ci/write-dockerfile.sh: Remove old .git file at the beginning, remov…

7fb0323

…e new .git file at the end

.github/workflows/*build*.yml: Survive GHA cache upload timeouts

5726013

build/pkgs/tdlib: Update to 0.9.2+a494876a8b168b50fc1dfca2f26b6e10878…

8086385

…158b6)

src/sage/graphs/graph_decompositions/sage_tdlib.cpp: Apply https://gi…

aa4a9b8

…tlab.archlinux.org/archlinux/packaging/packages/sagemath/-/blob/main/sagemath-tdlib-0.9.patch\?ref_type\=heads

Merge branch 'merge_ci_linux' into tdlib-0.9.2

2d8a10e

build/pkgs/tdlib: It is autotools, use release tarball 0.9.1

3763f97

build/pkgs/tdlib/dependencies: Add boost_cropped

8fbfa12

build/pkgs/tdlib/spkg-install.in: Use --without-python

ee601c0

build/pkgs/tdlib: Update to 0.9.3

e703a67

dcoudert approved these changes Jun 8, 2024

View reviewed changes

dimpase added s: needs info and removed s: positive review labels Jun 10, 2024

dimpase mentioned this pull request Jun 10, 2024

sagemath_environment must respect --enable-system-site-packages #38190

Closed

2 tasks

dimpase mentioned this pull request Jun 10, 2024

a PR claiming a fix must validate it #38192

Closed

dimpase added s: needs work and removed s: positive review labels Jun 10, 2024

dimpase added s: needs work and removed s: positive review labels Jun 11, 2024

saraedum added disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ and removed s: needs work labels Jun 11, 2024

dimpase added s: positive review and removed disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ labels Jun 11, 2024

dimpase mentioned this pull request Jun 13, 2024

test that #38159 is fixed #38214

Merged

vbraun merged commit c57cf01 into sagemath:develop Jun 22, 2024

github-actions bot removed the s: positive review label Jun 22, 2024

Uh oh!

Comments

Conversation

mkoeppe commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Checklist

⌛ Dependencies

Uh oh!

mkoeppe commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkoeppe commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felix-salfelder commented Jun 7, 2024

Uh oh!

mkoeppe commented Jun 7, 2024

Uh oh!

mkoeppe commented Jun 7, 2024

Uh oh!

felix-salfelder commented Jun 7, 2024 via email

Uh oh!

mkoeppe commented Jun 7, 2024

Uh oh!

mkoeppe commented Jun 7, 2024

Uh oh!

felix-salfelder commented Jun 7, 2024 via email

Uh oh!

mkoeppe commented Jun 7, 2024

Uh oh!

mkoeppe commented Jun 7, 2024

Uh oh!

dcoudert left a comment

Choose a reason for hiding this comment

Uh oh!

mkoeppe commented Jun 9, 2024

Uh oh!

dcoudert commented Jun 10, 2024

Uh oh!

mkoeppe commented Jun 10, 2024

Uh oh!

mkoeppe commented Jun 10, 2024

Uh oh!

mkoeppe commented Jun 10, 2024

Uh oh!

saraedum commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mkoeppe commented Jun 7, 2024 •

edited

Loading

mkoeppe commented Jun 7, 2024 •

edited

Loading

mkoeppe commented Jun 7, 2024 •

edited

Loading

saraedum commented Jun 11, 2024 •

edited

Loading