-
-
Notifications
You must be signed in to change notification settings - Fork 568
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
Remove "optional - gcc" from doctests #13533
Comments
Attachment: 13533_gcc_not_optional.patch.gz |
comment:2
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
Replying to @kcrisman:
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...) |
comment:4
Replying to @kcrisman:
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
(Also, I could have sworn that #6737 and #5094 would have gotten rid of most SageX references... of course there is still #5160 and |
comment:6
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
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
I guess I'm agnostic, like I said. I could easily go the other way. |
comment:9
Replying to @kcrisman:
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
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). |
comment:11
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
How does this ticket interact with #13540? |
comment:13
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 |
comment:14
Replying to @kcrisman:
I think this is it. |
comment:16
Okay, sounds good. |
comment:17
Replying to @jhpalmieri:
...as in positive_review? |
comment:18
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. |
Reviewer: Karl-Dieter Crisman, John Palmieri |
Merged: sage-5.5.beta0 |
Changed merged from sage-5.5.beta0 to sage-5.4.1.rc0 |
Some doctests are marked
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 withoutgcc
.Unfortunately, this exposes #12446, so we need "
# known bug
".Component: doctest coverage
Author: Jeroen Demeyer
Reviewer: Karl-Dieter Crisman, John Palmieri
Merged: sage-5.4.1.rc0
Issue created by migration from https://trac.sagemath.org/ticket/13533
The text was updated successfully, but these errors were encountered: