Skip to content
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

sagenb update to 1.0 #23066

Closed
dimpase opened this issue May 24, 2017 · 31 comments
Closed

sagenb update to 1.0 #23066

dimpase opened this issue May 24, 2017 · 31 comments

Comments

@dimpase
Copy link
Member

dimpase commented May 24, 2017

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

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

CC: @jdemeyer @jhpalmieri @kcrisman

Component: notebook

Author: Dima Pasechnik

Branch/Commit: 7b85dc2

Reviewer: John Palmieri, Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/23066

@dimpase dimpase added this to the sage-8.0 milestone May 24, 2017
@dimpase
Copy link
Member Author

dimpase commented May 24, 2017

comment:1

please review!

@jhpalmieri
Copy link
Member

comment:2

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.

@dimpase
Copy link
Member Author

dimpase commented May 25, 2017

comment:3

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.

@dimpase
Copy link
Member Author

dimpase commented May 25, 2017

comment:4

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).

@jdemeyer
Copy link

comment:5

Test failures are never harmless.

@dimpase
Copy link
Member Author

dimpase commented May 25, 2017

comment:6

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.

@dimpase
Copy link
Member Author

dimpase commented May 25, 2017

comment:7

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

@fchapoton
Copy link
Contributor

comment:8

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 ?

@fchapoton
Copy link
Contributor

comment:9

I just made a pull request on github

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 26, 2017

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 26, 2017

Changed commit from c29989b to 70fa3f8

@dimpase
Copy link
Member Author

dimpase commented May 26, 2017

comment:11

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).

@jhpalmieri
Copy link
Member

comment:12

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?

@jdemeyer
Copy link

jdemeyer commented Jun 2, 2017

Reviewer: John Palmieri, Jeroen Demeyer

@jdemeyer
Copy link

jdemeyer commented Jun 2, 2017

comment:13

Let me just check 2 things:

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

  2. That the tarball is correctly produced.

@jdemeyer
Copy link

jdemeyer commented Jun 2, 2017

comment:14

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 [comment: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.

@dimpase
Copy link
Member Author

dimpase commented Jun 2, 2017

comment:15

right, I shall bump up the version to 1.1.

@jdemeyer
Copy link

jdemeyer commented Jun 2, 2017

comment:16

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 3, 2017

Changed commit from 70fa3f8 to 3ce1714

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 3, 2017

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

@dimpase
Copy link
Member Author

dimpase commented Jun 3, 2017

comment:18

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

@dimpase

This comment has been minimized.

@jdemeyer
Copy link

jdemeyer commented Jun 3, 2017

comment:19

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.

  2. the filename should be fixed (1.0.1.tar.gz is not acceptable because it doesn't contain sagenb).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 3, 2017

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

7b85dc2bumping up the version

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 3, 2017

Changed commit from 3ce1714 to 7b85dc2

@dimpase
Copy link
Member Author

dimpase commented Jun 3, 2017

comment:21

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.

@dimpase

This comment has been minimized.

@jhpalmieri
Copy link
Member

comment:22

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

@jdemeyer
Copy link

jdemeyer commented Jun 6, 2017

comment:23

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.

@jdemeyer
Copy link

jdemeyer commented Jun 6, 2017

comment:24

I'm testing this again...

@vbraun
Copy link
Member

vbraun commented Jun 9, 2017

Changed branch from u/dimpase/sagenb10 to 7b85dc2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants