Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#10825 closed enhancement (fixed)

Make gap support SAGE_SPKG_INSTALL_DOCS

Reported by: jason Owned by: tbd
Priority: major Milestone: sage-4.7
Component: packages: standard Keywords:
Cc: wdjoyner@…, sal@… Merged in: sage-4.7.alpha1
Authors: Jason Grout Reviewers: David Kirkby
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Attachments (1)

10825.patch (1.3 KB) - added by jason 8 years ago.
FYI Only---already applied to spkg.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by jason

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by drkirkby

  • Reviewers set to David Kirkby
  • Status changed from needs_review to needs_work

See comments on #10828

comment:3 Changed 8 years ago by jason

  • Description modified (diff)
  • Status changed from needs_work to needs_review

New spkg up which should take care of David's points.

Changed 8 years ago by jason

FYI Only---already applied to spkg.

comment:4 Changed 8 years ago by drkirkby

I'm not in a position to check this now, but it would help if you could add the patch to the ticket, as not only does it making reviewing it much easier, but it is useful for others to quickly look at what changes were made.

Dave

comment:5 Changed 8 years ago by jason

Yep, there's a patch attached to the ticket now (for this and the other spkg tickets, as I finish them).

comment:6 follow-up: Changed 8 years ago by drkirkby

  • Cc wdjoyner@… sal@… added
  • Report Upstream changed from N/A to Reported upstream. Little or no feedback.
  • Status changed from needs_review to positive_review

The GAP change is implemented fine, so positive review.

However, the syntax of the HTML in GAP's documentation is incorrect. I just looked in index.htm and this immediately struck me:

<OL>
  <LI><A HREF="tut/chapters.htm">Tutorial</A>
  <LI><A HREF="ref/chapters.htm">Reference Manual</A>
  <LI><A HREF="prg/chapters.htm">Programming Tutorial</A>
  <LI><A HREF="ext/chapters.htm">Programming Reference Manual</A>
  <LI><A HREF="new/chapters.htm">New Features for Developers</A>
</OL>

There should be a </LI> on the end of list items, so the first would be:

  <LI><A HREF="tut/chapters.htm">Tutorial</A></LI>

but all 5 are missing, but that's obviously an upstream problem, so I have cc'ed the two people listed as the upstream contacts for GAP

comment:7 Changed 8 years ago by jason

I agree with you on all accounts.

comment:8 follow-up: Changed 8 years ago by was

From Steve Linton: "Thanks for the info. The documentation mechanisms are changing completely in the next release, and we will make sure they produce valid HTML.

Steve"

comment:9 in reply to: ↑ 8 Changed 8 years ago by drkirkby

  • Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. Developers acknowledge bug.

Replying to was:

From Steve Linton: "Thanks for the info. The documentation mechanisms are changing completely in the next release, and we will make sure they produce valid HTML.

Steve"

Thank you Steve for the prompt reply, and for William for forwarding it to trac.

Dave

comment:10 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

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

Replying to drkirkby:

The GAP change is implemented fine, so positive review.

However, the syntax of the HTML in GAP's documentation is incorrect. I just looked in index.htm and this immediately struck me:

<OL>
  <LI><A HREF="tut/chapters.htm">Tutorial</A>
  <LI><A HREF="ref/chapters.htm">Reference Manual</A>
  <LI><A HREF="prg/chapters.htm">Programming Tutorial</A>
  <LI><A HREF="ext/chapters.htm">Programming Reference Manual</A>
  <LI><A HREF="new/chapters.htm">New Features for Developers</A>
</OL>

This is actually correct HTML, the </LI> end tags are optional. So it's not a bug, even if the developers acknowledge it.

comment:12 Changed 8 years ago by jdemeyer

See also the W3C validator. There is an error for a misplaced </P> but it doesn't complain about the missing </LI>s.

comment:13 Changed 8 years ago by jason

huh. Thanks for the info. One more reason to like xhtml more than html, I guess!

comment:14 Changed 7 years ago by leif

I'm not sure what the following is supposed to do (or mean):

    # Build and install docs if requested
    if [ "x$SAGE_SPKG_INSTALL_DOCS" = xyes ] ; then
        if [ $? -ne 0 ]; then
            echo "Error building gap docs."
            exit 1
        fi
        mkdir -p $SAGE_ROOT/local/share/doc/gap
        cp -r src/doc/htm $SAGE_ROOT/local/share/doc/gap/html
    fi

Either there's a command missing to build the documentation, or we don't have to build it, in which case testing $? there is useless, since previous errors are catched above that block. (On the other hand, whether the creation of the directory and the copy command succeeded isn't checked at all, so maybe the if-block was supposed to be below the cp.)

I've currently changed this to

    # Build and install docs if requested:
    if [[ "$SAGE_SPKG_INSTALL_DOCS" = yes ]]; then
        # ?????
        if [[ $? -ne 0 ]]; then
            echo "Error building GAP docs."
            exit 1
        fi
        mkdir -p "$SAGE_LOCAL"/share/doc/gap &&
        cp -r src/doc/htm "$SAGE_LOCAL"/share/doc/gap/html
        if [[ $? -ne 0 ]]; then
            echo >&2 "Error copying GAP's HTML documentation."
            exit 1
        fi
    fi

Btw., the description in SPKG.txt (still) says the docs (as well as tests, which seem to be there again, too) have been removed from the spkg. The src/doc/ directory in contrast contains (subdirectories with) lots of LaTeX, HTML and probably other files.


I'm currently working on a GAP 4.4.12.p7 spkg for #7041, so perhaps leave comments (also) there.

comment:15 follow-ups: Changed 7 years ago by jason

We've talked a lot about a different way to go about building and installing the spkg docs (see #11197, for example) by having a separate spkg-install-docs script, sort of like how we have a separate spkg-check script.

I agree that the current test of $? doesn't make much sense; something is clearly missing.

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

Replying to jason:

We've talked a lot about a different way to go about building and installing the spkg docs (see #11197, for example)

Just wanted to cc me on that ticket, then realizing that I've already commented on it... m)

(To my excuse, this was 7 month ago.)


I agree that the current test of $? doesn't make much sense; something is clearly missing.

Well, currently the (apparently pre-built) HTML docs to get installed are there; I'm just not sure whether these are from upstream, or some Sage developer prepared them. I'll just add a note on that to SPKG.txt; I think we can then remove the other files (LaTeX sources etc.) from the spkg, at least for the moment. (Dima was recently talking about upgrading to the almost-released GAP 4.5 anyway.)

comment:17 in reply to: ↑ 15 Changed 7 years ago by jdemeyer

Replying to jason:

having a separate spkg-install-docs script, sort of like how we have a separate spkg-check script.

Actually, I like that!

Note: See TracTickets for help on using tickets.