Opened 4 years ago

Closed 4 years ago

#18508 closed task (fixed)

Various python 3 issues

Reported by: ohanar Owned by:
Priority: major Milestone: sage-6.8
Component: build Keywords:
Cc: vbraun, ncohen Merged in:
Authors: R. Andrew Ohana Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 83e849e (Commits) Commit: 83e849e176c5906cfc6a20a5d83129ee5ad62dff
Dependencies: Stopgaps:

Description

There are a number of really small issues when trying to build with python 3:

  1. sagenb is a marked as dependency of jmol, even though this is no longer the case
  2. sagenb is marked as a runtime dependency of sage, again this is no longer the case
  3. scons and sagenb don't work with python 3, so they should only be a standard package for python 2
  4. atlas needs a small change to make its build script work with python 3

Change History (37)

comment:1 Changed 4 years ago by ohanar

  • Status changed from new to needs_review

comment:2 Changed 4 years ago by ohanar

  • Dependencies set to #17607

comment:3 follow-up: Changed 4 years ago by vbraun

The notebook() command uses sagenb, so its a runtime dependency. See also #18512

I'm fine with EOL'ing SageNB with Python2, but you should probably post to sage-devel to see if anybody is interested in maintaining it.

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

Replying to vbraun:

The notebook() command uses sagenb, so its a runtime dependency.

No, it's not a run-time dependency, since Sage works without the Sage notebook. Of course, the notebook() command doesn't work, but that doesn't matter.

comment:5 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Unless you add support for the python2standard package type, this branch isn't going to work.

comment:6 follow-ups: Changed 4 years ago by vbraun

There are other packages that could be removed yet Sage would still starts up; of course some functionality is then missing. I don't see how sagenb is different. Define "works". If you mean "works enough to create the conway polynomial pickles" then I guess so.

The python2standard type is from #17607

comment:7 in reply to: ↑ 6 Changed 4 years ago by jdemeyer

Replying to vbraun:

Define "works".

This is actually documented in build/deps:

# Everything needed to start up Sage using "./sage".  Of course, not
# every part of Sage will work.  It does not include Maxima for example.

comment:8 Changed 4 years ago by jdemeyer

I guess a formal definition of "works" is that

./sage -c ""

has exit status 0.

comment:9 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:10 Changed 4 years ago by jdemeyer

I don't like this from #17607:

    if [ "$1" = all ]; then
        PKG_VAR=PYTHON
        if [ "$SAGE_PYTHON3" = yes ]; then
            PKG_NAME=python3
        else
            PKG_NAME=python2
        fi
        PKG_VERSION=$(newest_version $PKG_NAME)
        echo "$PKG_NAME $PKG_VERSION $PKG_VAR"
    fi

Just define PYTHON as alias of PYTHON2 or PYTHON3, analogous how build/install deals with GMP/MPIR and the toolchain. See the SAGE_MP_LIBRARY and TOOLCHAIN variables.

comment:11 follow-up: Changed 4 years ago by vbraun

We will have to remove python2 in the not so distant future so I don't see the value in winning any beauty contests in the meantime.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by jdemeyer

  • Cc ncohen added

Replying to vbraun:

We will have to remove python2 in the not so distant future

I think that future might be more distant than you think... but that is besides the point anyway.

I don't see the value in winning any beauty contests in the meantime.

This isn't just about beauty. It's about writing code which is easy to understand by doing things in a consistent way.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 4 years ago by ncohen

This isn't just about beauty. It's about writing code which is easy to understand by doing things in a consistent way.

+1

Plus we might also want to work on the build/ files to make them more readable, and that task is not made simpler if we have to clean "workarounds" on the way.

Nathann

P.S.: this does not exactly give the impression of a 'clean job'.

(GRRR|…)~/.Sage/build/pkgs$ head python3/checksums.ini -n 1
tarball=Python-VERSION.tar.gz
(GRRR|…)~/.Sage/build/pkgs$ head python2/checksums.ini -n 1
tarball=python-VERSION.tar.gz
Last edited 4 years ago by ncohen (previous) (diff)

comment:14 in reply to: ↑ 13 Changed 4 years ago by vbraun

Replying to ncohen:

Plus we might also want to work on the build/ files to make them more readable

Like strip out the Python 2.x stuff in a little bit over 4 years?

comment:15 in reply to: ↑ 6 Changed 4 years ago by jdemeyer

Replying to vbraun:

The python2standard type is from #17607

...and I will remove it in #18517 since it's a very ugly hack to add to the build system.

comment:16 Changed 4 years ago by jdemeyer

  • Dependencies changed from #17607 to #17607, #18517
  • Status changed from needs_review to needs_work

What exactly are you trying to achieve with making scons build on Python 2 only? How do you solve the problem that c_lib still requires scons to build?

comment:17 Changed 4 years ago by jdemeyer

For sagenb, you could instead edit build/pkgs/sagenb/spkg-install to skip the package on Python 3.

comment:18 follow-up: Changed 4 years ago by vbraun

scons doesn't support Python 3 at this point.

comment:19 in reply to: ↑ 18 Changed 4 years ago by jdemeyer

Replying to vbraun:

scons doesn't support Python 3 at this point.

I know, but read my question: How do you solve the problem that c_lib still requires scons to build? In other words: what's the gain from removing SCONS as standard package if it's still a dependency of csage?

comment:20 follow-up: Changed 4 years ago by vbraun

I don't claim to have solved that, but we might want to get rid of scons. I think Andrew did some work towards autotoolizing polybori. We might want to do the same for c_lib, though its not the only option.

comment:21 in reply to: ↑ 20 ; follow-up: Changed 4 years ago by jdemeyer

Replying to vbraun:

I don't claim to have solved that, but we might want to get rid of scons.

If it helps you, I propose to make scons an optional package. It will still be built by default since it's needed as dependency of polybori and c_lib.

We might want to do the same for c_lib, though its not the only option.

I assume you are aware of #17854?

comment:22 in reply to: ↑ 21 ; follow-up: Changed 4 years ago by ohanar

Replying to jdemeyer:

Replying to vbraun:

I don't claim to have solved that, but we might want to get rid of scons.

If it helps you, I propose to make scons an optional package. It will still be built by default since it's needed as dependency of polybori and c_lib.

Yes, I agree that this is cleaner than what I did. I'll make a patch sometime today updating this.

We might want to do the same for c_lib, though its not the only option.

I assume you are aware of #17854?

I've been basically assuming that #17854 will be done in the not too distant future, which will remove half of the scons dependency.

comment:23 Changed 4 years ago by git

  • Commit changed from 9d2af12245346ddaff1d6727fe968fdfbbc4cb84 to 1628a11e1ffbc062bf0d45558dabed30a3ad7e5f

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

1628a11fix scons and sagenb to work with #18517

comment:24 Changed 4 years ago by ohanar

  • Status changed from needs_work to needs_review

comment:26 in reply to: ↑ 22 Changed 4 years ago by jdemeyer

Replying to ohanar:

I've been basically assuming that #17854 will be done in the not too distant future

It's only ntl_wrap which remains (#18367). Moving ntl_wrap to the Sage library is relatively easy I think but a lot of boring work.

comment:27 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work
The content of "/usr/local/src/sage-config/build/pkgs/sagenb/type" must be 'base', 'standard', 'optional' or 'experimental'

(just set back the type to standard, see #18517)

comment:28 Changed 4 years ago by git

  • Commit changed from 1628a11e1ffbc062bf0d45558dabed30a3ad7e5f to 6fe3896ab7a602239f16628ec55242f163c6fe22

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

6fe3896make sagenb a standard package again

comment:29 Changed 4 years ago by ohanar

  • Status changed from needs_work to needs_review

comment:30 Changed 4 years ago by jdemeyer

This is probably good, but I'd rather wait for Sage 6.8.beta1 to formally review this.

comment:31 Changed 4 years ago by jdemeyer

  • Branch changed from u/ohanar/python3smallfixes to u/jdemeyer/python3smallfixes

comment:32 Changed 4 years ago by jdemeyer

  • Commit changed from 6fe3896ab7a602239f16628ec55242f163c6fe22 to 83e849e176c5906cfc6a20a5d83129ee5ad62dff
  • Dependencies #17607, #18517 deleted

New commits:

83e849eMerge tag '6.8.beta2' into t/18508/python3smallfixes

comment:33 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:34 Changed 4 years ago by vbraun

Reviewer name is missing

comment:35 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:36 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_work to positive_review

comment:37 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/python3smallfixes to 83e849e176c5906cfc6a20a5d83129ee5ad62dff
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.