Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#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:

Status badges

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 2 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/build/giac/isnan-isinf-bug
  • Cc dimpase added
  • Commit set to 832b97cd5d71f0d3acd8abbe87fbc4205eb96491
  • Status changed from new to needs_review

Also I have no idea what the FIR_LINUX macro here means. There is no documentation mentioning it and it is not defined anywhere.

comment:2 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Bad branch?

comment:3 follow-up: Changed 2 years ago by jdemeyer

This might also have been caused by #26787.

comment:4 Changed 2 years ago by git

  • Commit changed from 832b97cd5d71f0d3acd8abbe87fbc4205eb96491 to 2651e2029ae308c9ec0da72bcb7bd2d1ff981d26

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

2651e20Trac #27263: Added patch which fixes this issue in GIAC

comment:5 in reply to: ↑ 3 Changed 2 years ago by embray

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

2651e20Trac #27263: Added patch which fixes this issue in GIAC

comment:6 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

comment:7 Changed 2 years ago by embray

I still don't fully understand what #26787 does though so it's possible.

comment:8 Changed 2 years ago by embray

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: Changed 2 years ago by embray

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 2 years ago by jdemeyer

Why not just apply the patch? I'd rather not mess with environment variables.

comment:11 Changed 2 years ago by embray

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 2 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

LGTM

comment:13 follow-up: Changed 2 years ago by jdemeyer

I think you should bump the giac patchlevel such that this upgrade is actually tested.

comment:14 in reply to: ↑ 9 Changed 2 years ago by fbissey

Replying to embray:

Right now in sage-env-config I have

export 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's gnu++11 and not c++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 the spkg-install?

I'd rather have that little patch here.

comment:15 in reply to: ↑ 13 Changed 2 years ago by dimpase

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 2 years ago by vbraun

  • 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 2 years ago by slelievre

  • Cc frederichan parisse slelievre added
  • Commit 2651e2029ae308c9ec0da72bcb7bd2d1ff981d26 deleted
Note: See TracTickets for help on using tickets.