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

Upgrade to Cysignals 1.10.2 #27070

Closed
slel opened this issue Jan 17, 2019 · 37 comments
Closed

Upgrade to Cysignals 1.10.2 #27070

slel opened this issue Jan 17, 2019 · 37 comments

Comments

@slel
Copy link
Member

slel commented Jan 17, 2019

This is a blocker because it fixes #27384 which is required to run GAP on Cygwin.

CC: @antonio-rojas @embray @kiwifb @timokau @infinity0 @jdemeyer @slel @tobihan

Component: packages: standard

Keywords: upgrade, cysignals

Author: Jeroen Demeyer

Branch/Commit: a2ac9c7

Reviewer: Travis Scrimshaw, Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/27070

@jdemeyer
Copy link

comment:2

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.

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Upgrade to Cysignals 1.9.0 Upgrade to Cysignals 1.10.0 Mar 9, 2019
@timokau

This comment has been minimized.

@jdemeyer
Copy link

@jdemeyer
Copy link

Author: Jeroen Demeyer

@jdemeyer
Copy link

New commits:

2f60b3cUpgrade to Cysignals 1.10.0

@jdemeyer
Copy link

Commit: 2f60b3c

@timokau
Copy link
Contributor

timokau commented Mar 11, 2019

comment:8

Why is this a blocker?

@tscrim
Copy link
Collaborator

tscrim commented Mar 11, 2019

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Mar 11, 2019

comment:9

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

@jdemeyer
Copy link

comment:10

It's a blocker because it fixes #27384

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

comment:12

This is failing to build on OS X on conda-forge: conda-forge/cysignals-feedstock#19 (despite cysignals on OS X being tested on Travis CI).

@embray
Copy link
Contributor

embray commented Mar 12, 2019

comment:13

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.

@embray
Copy link
Contributor

embray commented Mar 12, 2019

comment:14

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?

@jdemeyer
Copy link

comment:15

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

@embray
Copy link
Contributor

embray commented Mar 12, 2019

comment:16

Okay, I just wasn't sure if the tests were built by default. But if so then that's a reasonable caution I suppose.

@jdemeyer

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 12, 2019

Changed commit from 2f60b3c to 04601fe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 12, 2019

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

04601feUpgrade to Cysignals 1.10.1

@vbraun
Copy link
Member

vbraun commented Mar 13, 2019

comment:21

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]       ^

@jdemeyer
Copy link

comment:22

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

@vbraun
Copy link
Member

vbraun commented Mar 13, 2019

comment:23

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

@vbraun
Copy link
Member

vbraun commented Mar 13, 2019

comment:24

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

@jhpalmieri
Copy link
Member

comment:25

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

@jdemeyer
Copy link

comment:26

Confirmed indeed with clang.

@jdemeyer
Copy link

comment:27

The problem is essentially that this doesn't compile:

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

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Upgrade to Cysignals 1.10.1 Upgrade to Cysignals 1.10.2 Mar 14, 2019
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 14, 2019

Changed commit from 04601fe to 14b1551

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 14, 2019

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

14b1551Upgrade to Cysignals 1.10.2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 14, 2019

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

a2ac9c7cysignals should be a normal dependency

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 14, 2019

Changed commit from 14b1551 to a2ac9c7

@jdemeyer
Copy link

comment:32

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

@embray
Copy link
Contributor

embray commented Mar 14, 2019

comment:33

I'm going to test this on Cygwin in a few minutes, but I don't expect any problems.

@embray
Copy link
Contributor

embray commented Mar 14, 2019

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Erik Bray

@vbraun
Copy link
Member

vbraun commented Mar 15, 2019

Changed branch from u/jdemeyer/upgrade_to_cysignals_1_10_0 to a2ac9c7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants