Opened 4 years ago
Closed 3 years ago
#27070 closed enhancement (fixed)
Upgrade to Cysignals 1.10.2
Reported by: | slelievre | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-8.7 |
Component: | packages: standard | Keywords: | upgrade, cysignals |
Cc: | arojas, embray, fbissey, gh-timokau, infinity0, jdemeyer, slelievre, thansen | Merged in: | |
Authors: | Jeroen Demeyer | Reviewers: | Travis Scrimshaw, Erik Bray |
Report Upstream: | N/A | Work issues: | |
Branch: | a2ac9c7 (Commits, GitHub, GitLab) | Commit: | a2ac9c78eb99858027bc12c60dc392e7ce3118a7 |
Dependencies: | Stopgaps: |
Description (last modified by )
This is a blocker because it fixes #27384 which is required to run GAP on Cygwin.
Change History (35)
comment:1 Changed 4 years ago by
- Type changed from PLEASE CHANGE to enhancement
comment:2 Changed 4 years ago by
comment:3 Changed 3 years ago by
- Description modified (diff)
- Summary changed from Upgrade to Cysignals 1.9.0 to Upgrade to Cysignals 1.10.0
comment:4 Changed 3 years ago by
- Description modified (diff)
comment:5 Changed 3 years ago by
- Branch set to u/jdemeyer/upgrade_to_cysignals_1_10_0
comment:6 Changed 3 years ago by
- Commit set to 2f60b3cc6fb650ee5a86bb7c14f0ed493eda378b
- Status changed from new to needs_review
New commits:
2f60b3c | Upgrade to Cysignals 1.10.0
|
comment:7 Changed 3 years ago by
- Priority changed from major to blocker
comment:8 Changed 3 years ago by
Why is this a blocker?
comment:9 Changed 3 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
I am not entirely sure. However, I am sending this off to the buildbots, so it is somewhat moot (but it would still be good to have an answer Jeroen ;)
).
comment:10 follow-up: ↓ 13 Changed 3 years ago by
It's a blocker because it fixes #27384
comment:11 Changed 3 years ago by
- Description modified (diff)
comment:12 Changed 3 years ago by
- Status changed from positive_review to needs_work
This is failing to build on OS X on conda-forge: https://github.com/conda-forge/cysignals-feedstock/pull/19 (despite cysignals on OS X being tested on Travis CI).
comment:13 in reply to: ↑ 10 Changed 3 years ago by
comment:14 follow-up: ↓ 15 Changed 3 years ago by
Since the build problem is just a question of building the tests on clang do we really care about that here, or do you need to make a 1.10.1?
comment:15 in reply to: ↑ 14 Changed 3 years ago by
Replying to embray:
Since the build problem is just a question of building the tests on clang do we really care about that here, or do you need to make a 1.10.1?
We do care if the problem with conda-forge also exists on the computers of Sage users. At this point, I don't know if the build failure is specific to Conda (and I guess it's not).
comment:16 Changed 3 years ago by
Okay, I just wasn't sure if the tests were built by default. But if so then that's a reasonable caution I suppose.
comment:17 Changed 3 years ago by
- Description modified (diff)
- Summary changed from Upgrade to Cysignals 1.10.0 to Upgrade to Cysignals 1.10.1
comment:18 Changed 3 years ago by
- Description modified (diff)
comment:19 Changed 3 years ago by
- Commit changed from 2f60b3cc6fb650ee5a86bb7c14f0ed493eda378b to 04601fea7a74c939ee158edafd32aed74e2fdc3e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
04601fe | Upgrade to Cysignals 1.10.1
|
comment:20 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:21 Changed 3 years ago by
- Status changed from needs_review to needs_work
I'm getting this with 1.10.0 on OSX, please check before setting back to positive review:
[fpylll-0.4.1dev] In file included from build/src/fpylll/fplll/integer_matrix.cpp:697: [fpylll-0.4.1dev] In file included from /Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/cysignals/struct_signals.h:45: [fpylll-0.4.1dev] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/10.0.0/include/stdatomic.h:75:26: error: reference to 'memory_order' is ambiguous [fpylll-0.4.1dev] void atomic_thread_fence(memory_order); [fpylll-0.4.1dev] ^ [fpylll-0.4.1dev] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/10.0.0/include/stdatomic.h:68:3: note: candidate found by name lookup is 'memory_order' [fpylll-0.4.1dev] } memory_order; [fpylll-0.4.1dev] ^ [fpylll-0.4.1dev] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/atomic:588:3: note: candidate found by name lookup is 'std::__1::memory_order' [fpylll-0.4.1dev] } memory_order; [fpylll-0.4.1dev] ^
comment:22 Changed 3 years ago by
Which OS X machine is that? I cannot reproduce it on your osx.fritz.box
(I tried cysignals-1.10.1 but probably that doesn't make a difference for this error).
comment:23 Changed 3 years ago by
Yes, that one. Whats the compiler invocation? I have
gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wno-unused -I/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/cysignals -I/Users/buildslave-sage/slave/sage_git/build/local/include -I/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/numpy/core/include -I/Users/buildslave-sage/slave/sage_git/build/local/include/python2.7 -c build/src/fpylll/fplll/integer_matrix.cpp -o build/temp.macosx-10.9-x86_64-2.7/build/src/fpylll/fplll/integer_matrix.o -std=c++11
comment:24 Changed 3 years ago by
You built sage's gcc, the buildbot doesn't and runs XCode clang
comment:25 Changed 3 years ago by
I get the same error on OS X (not building Sage's gcc).
comment:26 Changed 3 years ago by
Confirmed indeed with clang.
comment:27 Changed 3 years ago by
The problem is essentially that this doesn't compile:
using namespace std; #include <atomic> #include <stdatomic.h>
comment:28 Changed 3 years ago by
- Description modified (diff)
- Summary changed from Upgrade to Cysignals 1.10.1 to Upgrade to Cysignals 1.10.2
comment:29 Changed 3 years ago by
- Commit changed from 04601fea7a74c939ee158edafd32aed74e2fdc3e to 14b155156b09c7df85d3ed5bd6728a3847064d31
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
14b1551 | Upgrade to Cysignals 1.10.2
|
comment:30 Changed 3 years ago by
- Commit changed from 14b155156b09c7df85d3ed5bd6728a3847064d31 to a2ac9c78eb99858027bc12c60dc392e7ce3118a7
Branch pushed to git repo; I updated commit sha1. New commits:
a2ac9c7 | cysignals should be a normal dependency
|
comment:31 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:32 Changed 3 years ago by
I tested on that OS X machine that the cysignals test suite passes and that Sage builds.
comment:33 Changed 3 years ago by
I'm going to test this on Cygwin in a few minutes, but I don't expect any problems.
comment:34 Changed 3 years ago by
- Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Erik Bray
- Status changed from needs_review to positive_review
comment:35 Changed 3 years ago by
- Branch changed from u/jdemeyer/upgrade_to_cysignals_1_10_0 to a2ac9c78eb99858027bc12c60dc392e7ce3118a7
- Resolution set to fixed
- Status changed from positive_review to closed
Fine for me if somebody wants to do this, but there is no real reason for this upgrade. I mainly made the new release to answer to a Python bug.