Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#13533 closed enhancement (fixed)

Remove "optional - gcc" from doctests

Reported by: jdemeyer Owned by: mvngu
Priority: major Milestone: sage-5.4.1
Component: doctest coverage Keywords:
Cc: Merged in: sage-5.4.1.rc0
Authors: Jeroen Demeyer Reviewers: Karl-Dieter Crisman, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Some doctests are marked

# optional - gcc

but it seems reasonable to require gcc for doctests. After all, compiling Cython code is an integral part of Sage. Indeed, many doctests already fail without gcc.

Unfortunately, this exposes #12446, so we need "# known bug".

Attachments (1)

13533_gcc_not_optional.patch (5.0 KB) - added by jdemeyer 9 years ago.

Download all attachments as: .zip

Change History (22)

Changed 9 years ago by jdemeyer

comment:1 Changed 9 years ago by jdemeyer

  • Status changed from new to needs_review

comment:2 follow-ups: Changed 9 years ago by kcrisman

This seems to have a different mission than #11162. Maybe it's not obvious we should force people to have gcc to run doctests?

comment:3 in reply to: ↑ 2 Changed 9 years ago by jdemeyer

Replying to kcrisman:

Maybe it's not obvious we should force people to have gcc to run doctests?

The problem is that there are quite a few doctests already in Sage which require a C/C++/Fortran compiler. So we certainly could mark all those "# optional - foo compiler". But then a lot of tests would be missed in normal doctesting. For example, #12446 would not have happened if the "# optional - gcc" wasn't there.

I think it's not too much to ask for a user to have binutils installed in order to run Sage doctests. Why only binutils? If #13515 is merged (which will happen, since it's a blocker), we can ship all the needed compilers with Sage, which means the host system only needs to provide binutils (assembler, linker, archiver...)

Last edited 9 years ago by jdemeyer (previous) (diff)

comment:4 in reply to: ↑ 2 ; follow-up: Changed 9 years ago by jdemeyer

Replying to kcrisman:

Maybe it's not obvious we should force people to have gcc to run doctests?

A different —more pragmatic— answer is the following: if a user doesn't have a compiler toolchain, he is very unlikely to be a Sage developer. Why would a non-Sage-developer want to run all doctests?

comment:5 Changed 9 years ago by kcrisman

(Also, I could have sworn that #6737 and #5094 would have gotten rid of most SageX references... of course there is still #5160 and sage-sagex, see #9191, which I'm not sure how to fix easily... and the German and French tutorials :( which also refer to the now-defunct examples directory...)

comment:6 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by kcrisman

Maybe it's not obvious we should force people to have gcc to run doctests?

A different —more pragmatic— answer is the following: if a user doesn't have a compiler toolchain, he is very unlikely to be a Sage developer. Why would a non-Sage-developer want to run all doctests?

Because she is trying to help out? Just checking things work on an obscure platform? Wants to make sure their copy isn't corrupted somehow? I guess a better argument is your first one, that a number of tests already fail without gcc, so we should be consistent (which way, I'm agnostic on).

comment:7 Changed 9 years ago by jdemeyer

So you agree that make all gcc tests non-optional is the better option then? For me, the most important argument is to increase testing coverage.

comment:8 Changed 9 years ago by kcrisman

I guess I'm agnostic, like I said. I could easily go the other way.

comment:9 in reply to: ↑ 6 Changed 9 years ago by jdemeyer

Replying to kcrisman:

Just checking things work on an obscure platform?

The only way to have a working Sage without gcc is to download a binary. For this obscure platform, there won't be binaries, so you need gcc to build Sage in the first place.

comment:10 Changed 9 years ago by kcrisman

See my experience at #9191 for why I now agree with you. But can you post a link to the sage-devel or other discussion there was about this? I thought there was a brief one, but I can't find it now (probably it's not titled about gcc on Google groups).

Last edited 9 years ago by kcrisman (previous) (diff)

comment:11 Changed 9 years ago by kcrisman

This looks good. I just have to test it on Linux (due to the floating point issue) but presumably that's where you developed it so I doubt there will be problems.

comment:12 Changed 9 years ago by jhpalmieri

How does this ticket interact with #13540?

comment:13 follow-up: Changed 9 years ago by kcrisman

I'm having trouble doing any testing of 5.4.beta1 on sage.math - Cython is unavailable to me. I'm not sure why, it's just the usual sage.math binary. Anyway, if someone else can show this works there, I'm ok with it, but I'm reluctant to give final positive review otherwise. But it would suffice to have doctests run on Linux properly.


Naturally, if #13540 were to come about, that would be quite different and this would be partly unnecessary (though the known bug would still be). I knew there was something else - John, can you post a link to the thread where Robert discussed #13540 first? I just couldn't find it.

comment:14 in reply to: ↑ 13 Changed 9 years ago by jhpalmieri

Replying to kcrisman:

Naturally, if #13540 were to come about, that would be quite different and this would be partly unnecessary (though the known bug would still be). I knew there was something else - John, can you post a link to the thread where Robert discussed #13540 first? I just couldn't find it.

I think this is it.

comment:15 Changed 9 years ago by jdemeyer

I certainly think that #13540 is a good idea. However, I would prefer to merge #13533 which is ready now. When #13540 is ready, we can put the "optional" statements back in all the required places.

comment:16 follow-up: Changed 9 years ago by jhpalmieri

Okay, sounds good.

comment:17 in reply to: ↑ 16 Changed 9 years ago by jdemeyer

Replying to jhpalmieri:

Okay, sounds good.

...as in positive_review?

comment:18 Changed 9 years ago by jhpalmieri

  • Reviewers set to Karl-Dieter Crisman, John Palmieri
  • Status changed from needs_review to positive_review

I've now run tests on a few machines (including sage.math), so combined with what Karl-Dieter did, we can give this a positive review.

comment:19 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.4 to sage-5.5

comment:20 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.5.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:21 Changed 9 years ago by jdemeyer

  • Merged in changed from sage-5.5.beta0 to sage-5.4.1.rc0
  • Milestone changed from sage-5.5 to sage-5.4.1
Note: See TracTickets for help on using tickets.