Opened 9 years ago

Closed 8 years ago

#11793 closed defect (fixed)

autogenerate doc directories in sage-maketest

Reported by: jhpalmieri Owned by: mvngu
Priority: major Milestone: sage-5.1
Component: doctest coverage Keywords: sd40.5
Cc: leif, jdemeyer Merged in: sage-5.1.beta5
Authors: John Palmieri, Mike Hansen Reviewers: Mike Hansen, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jhpalmieri)

The script sage-maketest is supposed to test everything in the Sage library, but it says

"$SAGE_ROOT"/sage -t -sagenb "$@" "$SAGE_ROOT"/devel/sage/doc/common "$SAGE_ROOT"/devel/sage/doc/en "$SAGE_ROOT"/devel/sage/doc/fr  "$SAGE_ROOT"/devel/sage/sage 2>&1 | tee -a "$SAGE_TEST_LOG"

The relatively new directories .../doc/de and .../doc/ru are not there. We should autogenerate this list. The same goes for the script sage-apply-ticket.


Apply

Attachments (3)

trac_11793.v2.patch (2.7 KB) - added by jhpalmieri 9 years ago.
scripts repo
trac_11793-main.patch (777 bytes) - added by mhansen 9 years ago.
trac_11793-makefile.patch (783 bytes) - added by jdemeyer 8 years ago.
root repo

Download all attachments as: .zip

Change History (19)

comment:1 Changed 9 years ago by jhpalmieri

  • Status changed from new to needs_review

The attached patch is not perfect; a shell script guru could help here. See the comment on line 36 of the patched sage-maketest.

comment:2 Changed 9 years ago by jhpalmieri

  • Cc leif added

Leif: can you provide any feedback on this, especially the issue raised in line 36 of the patch to sage-maketest?

comment:3 in reply to: ↑ description Changed 9 years ago by leif

Replying to jhpalmieri:

The script sage-maketest is supposed to test everything in the Sage library, but it says

"$SAGE_ROOT"/sage -t -sagenb "$@" "$SAGE_ROOT"/devel/sage/doc/common "$SAGE_ROOT"/devel/sage/doc/en "$SAGE_ROOT"/devel/sage/doc/fr  "$SAGE_ROOT"/devel/sage/sage 2>&1 | tee -a "$SAGE_TEST_LOG"

The relatively new directories .../doc/de and .../doc/ru are not there.

Why not

"$SAGE_ROOT"/sage -t -sagenb "$@" \
    "$SAGE_ROOT"/devel/sage/doc/common \
    "$SAGE_ROOT"/devel/sage/doc/?? \
    "$SAGE_ROOT"/devel/sage/sage 2>&1 | tee -a "$SAGE_TEST_LOG"

or use [a-z][a-z} instead of ??.

These should always be two-letter country codes, ASCII.

comment:4 Changed 9 years ago by leif

We should IMHO stop to build (and hence test) all languages by default since doing so just wastes time and disk space and doesn't make sense.

Maybe always build "en", and also what's in SAGE_LANGUAGE or SAGE_LANGUAGES.

comment:5 follow-up: Changed 9 years ago by leif

You could also use

lang_dirs=`cd "$SAGE_ROOT"/devel/sage/doc/output/html/ && ls -1d ??`

(Again, perhaps use [a-z][a-z].)

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

Replying to leif:

You could also use

lang_dirs=`cd "$SAGE_ROOT"/devel/sage/doc/output/html/ && ls -1d ??`

(Again, perhaps use [a-z][a-z].)

I mean that gives you a list of the languages, without the full path.

Omit the cd and prepend the path to ?? if you need the full path.

comment:7 Changed 9 years ago by leif

If you want to avoid trouble with spaces in $f, use

docdirs="$docdirs '$f'"

But that's not necessary if you just use the shell's file pattern matching as given above.

comment:8 Changed 9 years ago by jhpalmieri

  • Description modified (diff)

Thanks for the suggestions. I think using [a-z][a-z] is clear and definitely accomplishes what we want. I think I'll change that in the Makefile, too. New patches attached.

As far as testing, I think we need to make sure to test everything, every time. This is one of the underlying philosophies of Sage. For example, if we don't test the Russian documentation by default, it might get tested very rarely, and not on a wide variety of platforms, and so failures might slip through. As you probably know, the documentation in the different languages is not very well synchronized, so testing the different languages may actually run different tests. It's also not bad to make sure testing keeps working with unicode files. Also, there isn't much documentation not in English right now, so this won't slow things down much.

For building, it doesn't take long to build the non-English documentation. If the reference manual ever gets translated, then it would make a lot of sense to only build the appropriate versions, but the rest of the docs are quick to build. Regardless, I don't want to address this here.

comment:9 Changed 9 years ago by jhpalmieri

  • Cc jdemeyer added

comment:10 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Why do we need sage-maketest in the first place? All other test targets (testall, testlong, ptest,...) simply use the top-level Makefile. Why does make test need to be special?

In any case, needs_work because the Makefile patch should be rebased to #11926 + #11959.

comment:11 follow-up: Changed 9 years ago by jdemeyer

Apologies, it does apply on top of #11926 and #11959 (it conflicts with #11969 but you don't need to rebase to that).

Line 31 of sage-apply-ticket: shouldn't ROOT be SAGE_ROOT?

Changed 9 years ago by jhpalmieri

scripts repo

comment:12 in reply to: ↑ 11 Changed 9 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Line 31 of sage-apply-ticket: shouldn't ROOT be SAGE_ROOT?

Yes, fixed.

comment:13 Changed 9 years ago by jhpalmieri

Rebased to Sage 5.0.beta8.

Changed 9 years ago by mhansen

comment:14 Changed 9 years ago by mhansen

  • Keywords sd40.5 added
  • Reviewers set to Mike Hansen

I'm okay with trac_11793-makefile.patch and trac_11793.v2.patch. I've added trac_11793-main.patch which should apply to the main repo and autogenerate LANGUAGES in build_options.py

John, could you look over this. Assuming that's okay, I think we can give this a positive review.

comment:15 Changed 9 years ago by jhpalmieri

  • Authors changed from John Palmieri to John Palmieri, Mike Hansen
  • Description modified (diff)
  • Reviewers changed from Mike Hansen to Mike Hansen, John Palmieri
  • Status changed from needs_review to positive_review

Your change looks good to me.

comment:16 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.1.beta5
  • Resolution set to fixed
  • Status changed from positive_review to closed

Changed 8 years ago by jdemeyer

root repo

Note: See TracTickets for help on using tickets.