Opened 3 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:

Status badges

Change History (35)

comment:1 Changed 3 years ago by embray

  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 3 years ago by jdemeyer

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.

comment:3 Changed 3 years ago by jdemeyer

  • 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 gh-timokau

  • Description modified (diff)

comment:5 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/upgrade_to_cysignals_1_10_0

comment:6 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to 2f60b3cc6fb650ee5a86bb7c14f0ed493eda378b
  • Status changed from new to needs_review

New commits:

2f60b3cUpgrade to Cysignals 1.10.0

comment:7 Changed 3 years ago by jdemeyer

  • Priority changed from major to blocker

comment:8 Changed 3 years ago by gh-timokau

Why is this a blocker?

comment:9 Changed 3 years ago by tscrim

  • 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: Changed 3 years ago by jdemeyer

It's a blocker because it fixes #27384

comment:11 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:12 Changed 3 years ago by jdemeyer

  • 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 embray

Replying to jdemeyer:

It's a blocker because it fixes #27384

Which itself is not really a blocker (though it is a bad issue) except to the extent that it's a prerequisite to fixing #27214 which could also be argued is not a blocker, but it is a pretty severe problem the more and more I look at it.

comment:14 follow-up: Changed 3 years ago by 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?

comment:15 in reply to: ↑ 14 Changed 3 years ago by jdemeyer

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 embray

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 jdemeyer

  • 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 jdemeyer

  • Description modified (diff)

comment:19 Changed 3 years ago by git

  • Commit changed from 2f60b3cc6fb650ee5a86bb7c14f0ed493eda378b to 04601fea7a74c939ee158edafd32aed74e2fdc3e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

04601feUpgrade to Cysignals 1.10.1

comment:20 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:21 Changed 3 years ago by vbraun

  • 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 jdemeyer

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 vbraun

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 vbraun

You built sage's gcc, the buildbot doesn't and runs XCode clang

comment:25 Changed 3 years ago by jhpalmieri

I get the same error on OS X (not building Sage's gcc).

comment:26 Changed 3 years ago by jdemeyer

Confirmed indeed with clang.

comment:27 Changed 3 years ago by jdemeyer

The problem is essentially that this doesn't compile:

using namespace std;
#include <atomic>
#include <stdatomic.h>

comment:28 Changed 3 years ago by jdemeyer

  • 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 git

  • Commit changed from 04601fea7a74c939ee158edafd32aed74e2fdc3e to 14b155156b09c7df85d3ed5bd6728a3847064d31

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

14b1551Upgrade to Cysignals 1.10.2

comment:30 Changed 3 years ago by git

  • Commit changed from 14b155156b09c7df85d3ed5bd6728a3847064d31 to a2ac9c78eb99858027bc12c60dc392e7ce3118a7

Branch pushed to git repo; I updated commit sha1. New commits:

a2ac9c7cysignals should be a normal dependency

comment:31 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:32 Changed 3 years ago by jdemeyer

I tested on that OS X machine that the cysignals test suite passes and that Sage builds.

comment:33 Changed 3 years ago by embray

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 embray

  • 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 vbraun

  • Branch changed from u/jdemeyer/upgrade_to_cysignals_1_10_0 to a2ac9c78eb99858027bc12c60dc392e7ce3118a7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.