Opened 3 years ago

Closed 3 years ago

#23066 closed enhancement (fixed)

sagenb update to 1.0

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.0
Component: notebook Keywords:
Cc: jdemeyer, jhpalmieri, kcrisman Merged in:
Authors: Dima Pasechnik Reviewers: John Palmieri, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 7b85dc2 (Commits) Commit: 7b85dc25f339d0988761c22da2f3dd4a2edc94c4
Dependencies: Stopgaps:

Description (last modified by dimpase)

This is sagenb update, incorporating changes accumulating since May 2016.

Tarball: http://users.ox.ac.uk/~coml0531/sage/sagenb-1.0.1.tar.bz2

Change History (26)

comment:1 Changed 3 years ago by dimpase

  • Cc jdemeyer jhpalmieri kcrisman added
  • Status changed from new to needs_review

please review!

comment:2 follow-up: Changed 3 years ago by jhpalmieri

See also #22787: do not build SageNB if SAGE_PYTHON3=yes. If this version fixes the Python 3 issues, then we should undo the change in #22787.

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

Replying to jhpalmieri:

See also #22787: do not build SageNB if SAGE_PYTHON3=yes. If this version fixes the Python 3 issues, then we should undo the change in #22787.

I propose we postpone dealing with this particular issue until Sage runs on python3.

comment:4 Changed 3 years ago by dimpase

I'd like to mention that after a first install of the new sagenb I sometimes have

sage -t --long --warn-long 54.6 local/lib/python2.7/site-packages/sagenb/notebook/worksheet.py
**********************************************************************
File "local/lib/python2.7/site-packages/sagenb/notebook/worksheet.py", line 477, in sagenb.notebook.worksheet.Worksheet.__lt__
Failed example:
    W1 <= W2
Expected:
    True
Got:
    False
**********************************************************************
File "local/lib/python2.7/site-packages/sagenb/notebook/worksheet.py", line 479, in sagenb.notebook.worksheet.Worksheet.__lt__
Failed example:
    W2 <= W1
Expected:
    False
Got:
    True

---something that I cannot reproduce after actually using sagenb. In fact even the re-run of the same test does not reproduce it. Not 100% sure how harmless these failures are. (I think they are harmless).

Last edited 3 years ago by dimpase (previous) (diff)

comment:5 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Test failures are never harmless.

comment:6 Changed 3 years ago by dimpase

If I reinstall the package then the test does not fail. Good luck debugging this, but I am not doing it.

As a workaround one can add running this test once in spkg-install and ignore its result.

comment:7 Changed 3 years ago by dimpase

  • Status changed from needs_work to needs_info

Has anyone seen this before? I stopped paying much attention to sagenb in 2012, so I cannot recall anything.

Last edited 3 years ago by dimpase (previous) (diff)

comment:8 Changed 3 years ago by chapoton

hmm, comparison of worksheet has been changed recently (to avoid using cmp)

This lives in sagenb/notebook/worksheet.py

I think at least that maybe there lacks __ne__ and maybe also @total_ordering ?

comment:9 Changed 3 years ago by chapoton

I just made a pull request on github

comment:10 Changed 3 years ago by git

  • Commit changed from c29989b0113afe41a12bb5e73754734a41fc3be9 to 70fa3f8da73a00053655f9e03e0e92d333f1b988

Branch pushed to git repo; I updated commit sha1. New commits:

664dcc9Merge branch 'u/dimpase/sagenb10' of trac.sagemath.org:sage into sagenb10
70fa3f8updates to properly compare worksheets

comment:11 Changed 3 years ago by dimpase

  • Status changed from needs_info to needs_review

OK, thanks to Frederic, the ordering of the worksheets is now fixed, and the doctests all pass too. (and the tarball is updated with these fixes, along with the checksum).

comment:12 Changed 3 years ago by jhpalmieri

This works for me. I haven't tested that thoroughly, but all tests pass, tests do not create the "SAGE_ROOT/home" directory any more, and the notebook seems to work. "Live" help works, too. Anyone object to a positive review?

comment:13 follow-up: Changed 3 years ago by jdemeyer

  • Reviewers set to John Palmieri, Jeroen Demeyer

Let me just check 2 things:

  1. A rebuild from scratch (i.e. after make distclean)
  1. That the tarball is correctly produced.

comment:14 in reply to: ↑ 13 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to jdemeyer:

Let me just check 2 things:

  1. A rebuild from scratch (i.e. after make distclean)

Doing this right now...

  1. That the tarball is correctly produced.

This is not looking good. Unless I'm missing something, the fixes mentioned in 11 are not in version 1.0 of SageNB. So the version number is misleading. Current SageNB master should be version 1.0.1 and then that tarball should be in Sage.

comment:15 Changed 3 years ago by dimpase

right, I shall bump up the version to 1.1.

comment:16 Changed 3 years ago by jdemeyer

If you want to use semantic versioning (not saying that you should), this is a bugfix, so it should be version 1.0.1

comment:17 Changed 3 years ago by git

  • Commit changed from 70fa3f8da73a00053655f9e03e0e92d333f1b988 to 3ce17147d5ac92c4f6fd87a27b9d4d880182fef9

Branch pushed to git repo; I updated commit sha1. New commits:

9f4c79cMerge branch 'u/dimpase/sagenb10' of trac.sagemath.org:sage into sagenb101
3ce1714bumping up the version, change to github tar.gz file

comment:18 Changed 3 years ago by dimpase

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

here is the bumped up version, repackaged so that the tarball is made by github.

comment:19 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

A git repository is not a source distribution. I suggest to use the dist.sh script to create the tarball.

That being said, it might be possible to install from a git repository, but then

  1. it should be clearly documented in SPKG.txt.
  1. the filename should be fixed (1.0.1.tar.gz is not acceptable because it doesn't contain sagenb).

comment:20 Changed 3 years ago by git

  • Commit changed from 3ce17147d5ac92c4f6fd87a27b9d4d880182fef9 to 7b85dc25f339d0988761c22da2f3dd4a2edc94c4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7b85dc2bumping up the version

comment:21 follow-up: Changed 3 years ago by dimpase

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

OK, I have followed your suggestion and used dist.sh now.

PS. Regarding the somewhat unfortunate naming scheme for github "releases", I can only say that the tar.gz file gets renamed to the right one if you download it using a browser.

comment:22 Changed 3 years ago by jhpalmieri

This also passes tests and works with my (cursory) usage after I built from scratch.

comment:23 in reply to: ↑ 21 Changed 3 years ago by jdemeyer

Replying to dimpase:

I can only say that the tar.gz file gets renamed to the right one if you download it using a browser.

My browser (GNU wget) didn't do that.

comment:24 Changed 3 years ago by jdemeyer

I'm testing this again...

comment:25 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:26 Changed 3 years ago by vbraun

  • Branch changed from u/dimpase/sagenb10 to 7b85dc25f339d0988761c22da2f3dd4a2edc94c4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.