Opened 7 years ago

Closed 3 years ago

#13540 closed enhancement (wontfix)

Automatically detect and test optional dependencies

Reported by: robertwb Owned by: mvngu
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: doctest framework Keywords:
Cc: jhpalmieri, iandrus, slelievre Merged in:
Authors: Reviewers: Kwankyu Lee, ​Samuel Lelièvre
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by leif)

I'd deferred actually fixing the broken doctests to #13884, #13885, and #2120 so we can start using this (and not have everything optional broken).


Superseded / fixed by #18558, #20182.

Attachments (2)

13540-optional-tests.2.patch (4.8 KB) - added by robertwb 7 years ago.
13540-disable-broken.patch (1.3 KB) - added by robertwb 7 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 7 years ago by jdemeyer

  • Authors set to Robert Bradshaw

Checking just for the existence of gcc is insufficient, you would have a lot of false positives with missing/broken assemblers/linkers... You actually need to compile something, preferably a shared library.

comment:2 Changed 7 years ago by jdemeyer

For the internet test: could you use www.sagemath.org instead of 173.194.33.3. In any case, an IP address is bad because

  1. you don't test DNS, which is required.
  2. it can change.

comment:3 Changed 7 years ago by jdemeyer

Also please add a check for g++ and gfortran.

comment:4 Changed 7 years ago by jdemeyer

Instead of relying on which, simply execute the command and catch OSError.

comment:5 Changed 7 years ago by jdemeyer

Command lines for gcc, g++, gfortran compiling a shared library from /dev/null:

gcc -shared -fpic -x c /dev/null -o /dev/null
g++ -shared -fpic -x c++ /dev/null -o /dev/null
gfortran -cpp -ffree-form -shared -fpic -x f95 /dev/null -o /dev/null

I have no idea how to do any of this with other compilers such as clang.

comment:6 Changed 7 years ago by jdemeyer

I think it would be good to implement either caching or lazy evaluation (only check when actually encountering # optional - foo). Otherwise, somebody with a slow internet connection could see a substantial time added to every doctest run.

comment:7 Changed 7 years ago by jdemeyer

For programs like magma, does Sage require a specific version? If so, you should check for that version.

comment:8 Changed 7 years ago by robertwb

I would be surprised if many people that have gcc have a broken gcc, but it couldn't hurt to check (and g++, etc.) Same with internet--it's rare for people to be connected but not be able to resolve DNS. (If this is lazy, latency less of an issue.)

Good points. I'll refactor this to make things lazy and otherwise improve the patch.

comment:9 Changed 7 years ago by jhpalmieri

  • Cc jhpalmieri added

comment:10 Changed 7 years ago by iandrus

  • Cc iandrus added

Changed 7 years ago by robertwb

comment:11 Changed 7 years ago by robertwb

  • Status changed from new to needs_review

Apply only 13540-optional-tests.2.patch

comment:12 Changed 7 years ago by robertwb

Apply 13540-optional-tests.2.patch to sage-local repo.

comment:13 Changed 7 years ago by vbraun

The patch is great but a great number of the # optional - internet doctests fail because they haven't been doctested in a while... Any opinion on how to proceeed?

Changed 7 years ago by robertwb

comment:14 Changed 7 years ago by robertwb

  • Description modified (diff)

comment:15 follow-up: Changed 7 years ago by leif

IMHO we shouldn't hardcode the names of the executables (gcc, g++, gfortran etc.), but respect for example CC, CXX, FC/F77/F95.

(Also wonder whether "# optional - gcc" really means GCC, or some C compiler, perhaps at least capable of compiling Cython-generated code.)

comment:16 in reply to: ↑ 15 Changed 7 years ago by leif

Replying to leif:

IMHO we shouldn't hardcode the names of the executables (gcc, g++, gfortran etc.), but respect for example CC, CXX, FC/F77/F95.

As an alternative, we might put their settings "during build" (whatever that means) into dummy/wrapper scripts in $SAGE_LOCAL/bin/. (I don't really have an opion on that, but tend to be against such. It might be convenient to "ordinary" users, or perhaps for automated testing, but annoying or confusing to developers.)

comment:17 follow-up: Changed 7 years ago by robertwb

For all intents and purposes, at the moment, gcc really means gcc. And building a Cython extension with a different compiler than Python was compiled with (and the relevant libraries, for C++ in particular) gets really messy.

In any case, the point of this ticket is to introduce a framework that lets us test optional doctests automatically, as they the current setup means they're never tested and so quickly broken. We could do something more sophisticated with CC, etc. but gcc, at the very least, provides a common enough dependency to actually exercise this code for most (probably all) developers (and it's not near as brittle as others).

comment:18 in reply to: ↑ 17 Changed 7 years ago by leif

Replying to robertwb:

For all intents and purposes, at the moment, gcc really means gcc. And building a Cython extension with a different compiler than Python was compiled with (and the relevant libraries, for C++ in particular) gets really messy.

We might then even use Python's / distutils' settings for testing tool availability (although there's no variable for the Fortran compiler AFAIK). Not sure what to do with bdists (in case they don't contain a pre-built GCC as well).


In any case, the point of this ticket is to introduce a framework that lets us test optional doctests automatically, as they the current setup means they're never tested and so quickly broken. We could do something more sophisticated with CC, etc. but gcc, at the very least, provides a common enough dependency to actually exercise this code for most (probably all) developers (and it's not near as brittle as others).

Yes. Nevertheless, I don't think I'm the only one that almost always sets CC, CXX etc., in order to use different versions of GCC.

Support for other compilers (e.g. clang), probably also flags to pass, is certainly out of the scope of this ticket. (It would be convenient to have something like config.status, i.e., a mechanism for storing and restoring the build environment, and/or also a ".sagerc" local / specific to the Sage installation, one overriding the other...)

comment:19 Changed 7 years ago by jdemeyer

How is this supposed to interact with the --optional and --only-optional command line options? I have a hard time understanding the new code.

comment:20 Changed 7 years ago by roed

  • Component changed from doctest to doctest framework

comment:21 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This would obviously need to be rebased to #12415.

comment:22 Changed 6 years ago by ncohen

(just to follow what's happening. This ticket would be a nice change indeed O_o)

comment:23 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:24 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:25 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:26 Changed 6 years ago by leif

Magma doctest failures now get fixed at #16322.

comment:27 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:28 Changed 5 years ago by ncohen

Hello everybody,

A branch that does that has been created at #18558 (needs review)

Nathann

Last edited 5 years ago by ncohen (previous) (diff)

comment:29 Changed 4 years ago by klee

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

This is now "duplicate/wontfix" as the replacement #20182 is now merged.

comment:30 follow-up: Changed 3 years ago by slelievre

  • Cc slelievre added
  • Summary changed from Automatically detect and test optional dependencies. to Automatically detect and test optional dependencies

Is this ticket solved by the other tickets mentioned in the last comments? Does any comment in the discussion beg for a follow-up ticket?

comment:31 in reply to: ↑ 30 Changed 3 years ago by klee

Replying to slelievre:

Is this ticket solved by the other tickets mentioned in the last comments?

Yes.

Does any comment in the discussion beg for a follow-up ticket?

I don't think so. Most of the comments in this discussion are 4 years old, and so perhaps invalid now.

comment:32 Changed 3 years ago by slelievre

  • Status changed from needs_review to positive_review

comment:33 Changed 3 years ago by leif

  • Authors Robert Bradshaw deleted
  • Reviewers set to Kwankyu Lee, ​Samuel Lelièvre

4-year old => invalid

ROFL.

comment:34 Changed 3 years ago by leif

  • Description modified (diff)

comment:35 Changed 3 years ago by embray

  • Resolution set to wontfix
  • Status changed from positive_review to closed

Determined to be invalid/duplicate/wontfix (closing as "wontfix" as a catch-all resolution).

Note: See TracTickets for help on using tickets.