#27263 closed defect (fixed)
Upgrade to giac 1.5 fails to build with older libstdc++
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-8.7 |
Component: | packages: standard | Keywords: | |
Cc: | dimpase, frederichan, parisse, slelievre | Merged in: | |
Authors: | Erik Bray | Reviewers: | Dima Pasechnik |
Report Upstream: | Not yet reported upstream; Will do shortly. | Work issues: | |
Branch: | 2651e20 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
#26315 broke building giac on some systems that have a old-ish libstdc++. It is completely mysterious to me why this broke when it worked before, and without a repository it's very difficult to determine what change between the versions is responsible, as there is little difference between the two versions in the relevant code.
Nevertheless, it can be fixed by explicitly using std::isnan
and std::isinf
so that there is no risk of them conflicting with the libc math.h equivalents thereof.
Change History (17)
comment:1 Changed 3 years ago by
- Branch set to u/embray/build/giac/isnan-isinf-bug
- Cc dimpase added
- Commit set to 832b97cd5d71f0d3acd8abbe87fbc4205eb96491
- Status changed from new to needs_review
comment:3 follow-up: ↓ 5 Changed 3 years ago by
This might also have been caused by #26787.
comment:4 Changed 3 years ago by
- Commit changed from 832b97cd5d71f0d3acd8abbe87fbc4205eb96491 to 2651e2029ae308c9ec0da72bcb7bd2d1ff981d26
Branch pushed to git repo; I updated commit sha1. New commits:
2651e20 | Trac #27263: Added patch which fixes this issue in GIAC
|
comment:5 in reply to: ↑ 3 Changed 3 years ago by
- Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.
Replying to jdemeyer:
This might also have been caused by #26787.
According to my research this occurs with or without -std=c++11
but I haven't explicitly tested that.
I will also try to bring it up upstream in case there is any idea.
New commits:
2651e20 | Trac #27263: Added patch which fixes this issue in GIAC
|
comment:6 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:7 Changed 3 years ago by
I still don't fully understand what #26787 does though so it's possible.
comment:8 Changed 3 years ago by
Okay, confirmed that it is #26787 after all. If I just put unset CXX
in the spkg-install for giac it builds. Something like that might be better than patching...
comment:9 follow-up: ↓ 14 Changed 3 years ago by
Right now in sage-env-config
I have
export CONFIGURED_CXX="g++ -std=gnu++11 -std=gnu++11"
I don't know why the -std=gnu++11
is repeated. Conceivably it might also matter that it's gnu++11
and not c++11
. Perhaps this older gcc is not fully c++11-compatible?
In any case, what would be the best approach here, do you think? Would it be reasonable to just unset CXX
in the spkg-install
?
comment:10 Changed 3 years ago by
Why not just apply the patch? I'd rather not mess with environment variables.
comment:11 Changed 3 years ago by
I guess, I don't really know what impact the patch has on other systems. But it does seem clear to me: Just explicitly use the isnan
in the std
namespace. I assume there's no reason that should go wrong...
comment:12 Changed 3 years ago by
- Reviewers set to Dima Pasechnik
- Status changed from needs_review to positive_review
LGTM
comment:13 follow-up: ↓ 15 Changed 3 years ago by
I think you should bump the giac
patchlevel such that this upgrade is actually tested.
comment:14 in reply to: ↑ 9 Changed 3 years ago by
Replying to embray:
Right now in
sage-env-config
I haveexport CONFIGURED_CXX="g++ -std=gnu++11 -std=gnu++11"
The only way I think you can end up with that is for CXX
to be g++ -std=gnu++11
at configuration time. The macro will then happily add another one because it is fairly stupid.
I don't know why the
-std=gnu++11
is repeated. Conceivably it might also matter that it'sgnu++11
and notc++11
.
Don't think so but I am a C++ bigwig.
Perhaps this older gcc is not fully c++11-compatible?
Most certainly. Before gcc-5 C++11 support was experimental and the implementation was not fixed in stone.
In any case, what would be the best approach here, do you think? Would it be reasonable to just
unset CXX
in thespkg-install
?
I'd rather have that little patch here.
comment:15 in reply to: ↑ 13 Changed 3 years ago by
Replying to jdemeyer:
I think you should bump the
giac
patchlevel such that this upgrade is actually tested.
In theory yes, but this only matters for systems that otherwise simply cannot built giac, so you don't need to do this bumping for all practical purposes.
comment:16 Changed 3 years ago by
- Branch changed from u/embray/build/giac/isnan-isinf-bug to 2651e2029ae308c9ec0da72bcb7bd2d1ff981d26
- Resolution set to fixed
- Status changed from positive_review to closed
comment:17 Changed 3 years ago by
- Cc frederichan parisse slelievre added
- Commit 2651e2029ae308c9ec0da72bcb7bd2d1ff981d26 deleted
Also I have no idea what the
FIR_LINUX
macro here means. There is no documentation mentioning it and it is not defined anywhere.