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

build/pkgs/tdlib: Update to 0.9.3 #38163

Merged
merged 21 commits into from
Jun 22, 2024
Merged

build/pkgs/tdlib: Update to 0.9.3 #38163

merged 21 commits into from
Jun 22, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Jun 7, 2024

📝 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

Matthias Koeppe added 14 commits June 2, 2024 22:50
…t from build.yml, use ptest-nodoc to avoid duplicate doc-html build
@mkoeppe mkoeppe changed the title build/pkgs/tdlib: Update to 0.9.2 build/pkgs/tdlib: Update to 0.9.1 Jun 7, 2024
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 7, 2024

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
Copy link
Contributor Author

mkoeppe commented Jun 7, 2024

@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
Copy link

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
Copy link
Contributor Author

mkoeppe commented Jun 7, 2024

Do you think we can also get rid of boost_system?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 7, 2024

Wait, is this whole thing header-only?

@felix-salfelder
Copy link

felix-salfelder commented Jun 7, 2024 via email

@dcoudert
Copy link
Contributor

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 10, 2024

Update PRs are not in charge of adding such tests

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 10, 2024

Just open a PR for adding this.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 10, 2024

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

@saraedum
Copy link
Member

saraedum commented Jun 11, 2024

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

@saraedum 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 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
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

- FIxes sagemath#30813

### 📝 Checklist

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

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

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- Depends on sagemath#38144 (merged here for testing)
    
URL: sagemath#38163
Reported by: Matthias Köppe
Reviewer(s): David Coudert
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
    
This will fix sagemath#38159

Add a missing test

### 📝 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.
- [ x] 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#38163
    
URL: sagemath#38214
Reported by: Dima Pasechnik
Reviewer(s): David Coudert
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

- FIxes sagemath#30813

### 📝 Checklist

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

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

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- Depends on sagemath#38144 (merged here for testing)
    
URL: sagemath#38163
Reported by: Matthias Köppe
Reviewer(s): David Coudert
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
    
This will fix sagemath#38159

Add a missing test

### 📝 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.
- [ x] 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#38163
    
URL: sagemath#38214
Reported by: Dima Pasechnik
Reviewer(s): David Coudert
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 17, 2024
    
This will fix sagemath#38159

Add a missing test

### 📝 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.
- [ x] 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#38163
    
URL: sagemath#38214
Reported by: Dima Pasechnik
Reviewer(s): David Coudert
@vbraun vbraun merged commit c57cf01 into sagemath:develop Jun 22, 2024
53 of 60 checks passed
@mkoeppe mkoeppe deleted the tdlib-0.9.2 branch June 22, 2024 20:15
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.

try to upgrade tdlib
6 participants