Opened 6 years ago

Closed 6 years ago

#14480 closed task (fixed)

switch sage to the new directory layout

Reported by: ohanar Owned by: tbd
Priority: major Milestone: sage-6.0
Component: distribution Keywords:
Cc: jdemeyer, saraedum Merged in:
Authors: R. Andrew Ohana Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: u/ohanar/build_system Commit: cd237e726293c48288016aa2a9cc80aa75a3bd20
Dependencies: #13015, #14781 Stopgaps:

Description (last modified by ohanar)

To switch to a unified repository we need to include some information that are currently in the spkg tarballs, which requires modifying the sage's directory structure. Since we are switching workflows at the same time, it is an ideal time to rethink the directory structure as a whole.

The proposed significant changes

spkg -> build
devel/sage -> src
devel/ext -> src/ext
sage_scripts -> src/bin
spkg/bin -> src/bin

devel/sagenb -> /dev/null (is now considered a typical upstream python library)
/dev/null -> build/pkgs/* (these are now the spkg repositories)

Full changes are described at http://wiki.sagemath.org/WorkflowSEP.

The script used to create the git repository is consolidate-repos.sh, which depends on configuration.sh, git-filter-branch, and the fast-export directory (all located at https://github.com/sagemath/sage-workflow). They require bash4, git 1.8.x, and python 2.7 to run (at the very least, possibly more).

Change History (55)

comment:1 Changed 6 years ago by ohanar

  • Status changed from new to needs_review

This can really only be reviewed on the new trac instance. For now, the branch field on http://trac.tangentspace.org/ticket/13015 links to the list of changes (to actually see the changes click the view changes button -- since this includes some massive changes, it might take awhile to load the side-by-side diff).

comment:2 Changed 6 years ago by ohanar

  • Cc saraedum added

comment:3 Changed 6 years ago by ohanar

  • Milestone changed from sage-5.10 to sage-6.0

comment:4 Changed 6 years ago by ohanar

  • Summary changed from switch sage to the new layout to switch sage to the new directory layout

comment:5 Changed 6 years ago by jdemeyer

Where is the script that you used to automatically create the git repo from the hg repos? That should be reviewed too and kept for reference.

comment:6 Changed 6 years ago by ohanar

  • Description modified (diff)

comment:7 Changed 6 years ago by ohanar

  • Dependencies changed from #13015 to #13015, #14781

comment:8 Changed 6 years ago by ohanar

  • Branch set to u/ohanar/build_system

comment:9 follow-up: Changed 6 years ago by mmezzarobba

  • Status changed from needs_review to needs_info

A few small issues with this branch. I am not sure which of these should actually be fixed here, so I'm marking the ticket as need_info rather that need_work.

  • There remain references to src/sage/misc/hg.py (which was removed as part of 48ff645017639d70d9b4a7107ac7fd98d8a871b5) in src/sage/doctest/control.py.
sage -t --long src/sage/doctest/control.py
**********************************************************************
File "src/sage/doctest/control.py", line 434, in sage.doctest.control.DocTestController.add_files
Failed example:
    DC.add_files()
Exception raised:
    Traceback (most recent call last):
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 486, in _run
        self.execute(example, compiled, test.globs)
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 845, in execute
        exec compiled in globs
      File "<doctest sage.doctest.control.DocTestController.add_files[10]>", line 1, in <module>
        DC.add_files()
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/control.py", line 458, in add_files
        from sage.misc.hg import hg_sage
    ImportError: No module named hg
**********************************************************************
1 item had failures:
   1 of  16 in sage.doctest.control.DocTestController.add_files
    [162 tests, 1 failure, 1.97 s]
  • I also get a few more doctest failures such as the one below due to a missing import that seems to be fixed in the current git version of sagenb.
**********************************************************************
File "local/lib/python2.7/site-packages/sagenb-0.10.4-py2.7.egg/sagenb/misc/misc.py", line 44, in sagenb.misc.misc.print_open_msg
Failed example:
    sage.server.misc.print_open_msg('localhost', 8080, True)
Exception raised:
    Traceback (most recent call last):
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 486, in _run
        self.execute(example, compiled, test.globs)
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 845, in execute
        exec compiled in globs
      File "<doctest sagenb.misc.misc.print_open_msg[0]>", line 1, in <module>
        sage.server.misc.print_open_msg('localhost', Integer(8080), True)
    AttributeError: 'module' object has no attribute 'misc'
**********************************************************************
  • Finally, trying to run the notebook produces the following error:
sage: notebook()
The notebook files are stored in: sage_notebook.sagenb
**************************************************
*                                                *
* Open your web browser to http://localhost:8080 *
*                                                *
**************************************************
Executing twistd  --pidfile="sage_notebook.sagenb/sagenb.pid" -ny "sage_notebook.sagenb/twistedconf.tac"
Unhandled Error
Traceback (most recent call last):
  File "/home/marc/co/sage/local/lib/python2.7/site-packages/Twisted-12.3.0-py2.7-linux-x86_64.egg/twisted/application/app.py", line 652, in run
    runApp(config)
  File "/home/marc/co/sage/local/lib/python2.7/site-packages/Twisted-12.3.0-py2.7-linux-x86_64.egg/twisted/scripts/twistd.py", line 23, in runApp
    _SomeApplicationRunner(config).run()
  File "/home/marc/co/sage/local/lib/python2.7/site-packages/Twisted-12.3.0-py2.7-linux-x86_64.egg/twisted/application/app.py", line 386, in run
    self.application = self.createOrGetApplication()
  File "/home/marc/co/sage/local/lib/python2.7/site-packages/Twisted-12.3.0-py2.7-linux-x86_64.egg/twisted/application/app.py", line 451, in createOrGetApplication
    application = getApplication(self.config, passphrase)
--- <exception caught here> ---
  File "/home/marc/co/sage/local/lib/python2.7/site-packages/Twisted-12.3.0-py2.7-linux-x86_64.egg/twisted/application/app.py", line 462, in getApplication
    application = service.loadApplication(filename, style, passphrase)
  File "/home/marc/co/sage/local/lib/python2.7/site-packages/Twisted-12.3.0-py2.7-linux-x86_64.egg/twisted/application/service.py", line 405, in loadApplication
    application = sob.loadValueFromFile(filename, 'application', passphrase)
  File "/home/marc/co/sage/local/lib/python2.7/site-packages/Twisted-12.3.0-py2.7-linux-x86_64.egg/twisted/persisted/sob.py", line 210, in loadValueFromFile
    exec fileObj in d, d
  File "sage_notebook.sagenb/twistedconf.tac", line 15, in <module>
    import base as flask_base
exceptions.ImportError: No module named base

Failed to load application: No module named base

---------------------------------------------------------------------------
error                                     Traceback (most recent call last)
<ipython-input-1-3728cb3d7c7d> in <module>()
----> 1 notebook()

/home/marc/co/sage/local/lib/python2.7/site-packages/sagenb-0.10.4-py2.7.egg/sagenb/notebook/notebook_object.pyc in __call__(self, *args, **kwds)
    221     """
    222     def __call__(self, *args, **kwds):
--> 223         return self.notebook(*args, **kwds)
    224 
    225     notebook = run_notebook.notebook_run

/home/marc/co/sage/local/lib/python2.7/site-packages/sagenb-0.10.4-py2.7.egg/sagenb/notebook/run_notebook.pyc in notebook_run(self, directory, port, interface, port_tries, secure, reset, accounts, openid, server_pool, ulimit, timeout, upload, automatic_login, start_path, fork, quiet, server, profile, subnets, require_login, open_viewer, address)
    626     os.chdir(cwd)
    627     if e == 256:
--> 628         raise socket.error
    629 
    630 def get_admin_passwd():

error:

comment:10 in reply to: ↑ 9 Changed 6 years ago by saraedum

  • Status changed from needs_info to needs_review

Thanks for having a look.

Replying to mmezzarobba:

  • There remain references to src/sage/misc/hg.py (which was removed as part of 48ff645017639d70d9b4a7107ac7fd98d8a871b5) in src/sage/doctest/control.py.

This is now #14954.

  • I also get a few more doctest failures such as the one below due to a missing import that seems to be fixed in the current git version of sagenb.

This is fixed by #14330.

I believe that these issues are not related to consolidate-repos.sh, so I'll put this back to needs_review. These two tickets are part of the metaticket #13015.

comment:11 Changed 6 years ago by tkluck

comment:12 follow-up: Changed 6 years ago by tkluck

  • Status changed from needs_review to needs_info

The following is my review for the changes between master and u/ohanar/build_system. I'm assuming that there are no other changes apart from the directory structure between the old hg repositories and master.

Overall, I'm impressed by the amount of work this has been. My review contains mostly questions that you can probably just answer or address with minor patches.

Maybe it makes sense for at least one more person to go through this diff.

diff --git a/Makefile b/Makefile
 doc-clean:
-	@echo "Deleting devel/sage/doc/output..."
-	rm -rf devel/sage/doc/output
+	@echo "Deleting generated docs..."
+	rm -rf src/doc/en/reference/*/sage
+	rm -rf src/doc/en/reference/*/sagenb
+	rm -rf src/doc/en/reference/sage
+	rm -rf src/doc/en/reference/sagenb
+	rm -rf src/doc/output

why do we clean more directories than before?

+
+bdist-clean: clean
+	@echo "Deleting miscellaneous artifacts generated by build system ..."
 	rm -rf logs
-	rm -f spkg/parallel_make.cfg
 	rm -rf dist

What happened to this parallel_make.cfg file?

Do I understand correctly that, and is it okay that the micro_release code now only strips the binaries and does not remove build directories?

diff --git a/build/deps b/build/deps
new file mode 100644
index 0000000..2d2d19c
--- /dev/null
+++ b/build/deps
@@ -0,0 +1,523 @@
+###############################################################################
+# This file ($SAGE_ROOT/spkg/standard/deps) will be copied into
+# $SAGE_ROOT/spkg/Makefile by $SAGE_ROOT/spkg/install
+###############################################################################

There's still a reference to the spkg directory here.

diff --git a/build/gen_html b/build/gen_html
deleted file mode 100755
index 6dbad06..0000000
--- a/build/gen_html
+++ /dev/null

What did this script do? Why don't we need it anymore?

diff --git a/build/install b/build/install
index 780fe2a..bb14f76 100755
--- a/build/install
+++ b/build/install
@@ -99,11 +95,6 @@ if [ -d "$SAGE_ROOT/data" ] && [ ! -h "$SAGE_ROOT/data" ]; then
     rm -rf "$SAGE_ROOT/data"
 fi
-# Symlink old SAGE_DATA to SAGE_SHARE for optional/experimental
-# packages that haven't yet been updated.
-rm -f "$SAGE_ROOT/data"
-ln -s local/share "$SAGE_ROOT/data"
-

Should we make sure that this does not break any optional/experimental packages? Personally, I'm fine with any package that installs anything into this data directory, which I'm guessing has been deprecated for some time now. (how long?)

@@ -352,25 +343,17 @@ newest_version_base() {
 }
 
 # Usage: newest_version $pkg
-# Print version number of latest (according to modification time)
-# standard or optional package $pkg
+# Print version number of latest standard package $pkg
 newest_version() {
 <SNIP>

This reminds me: will we, at some point, test the installation of the optional packages into the git framework? And, since we're already distributing them officially, couldn't we just as well put their metadata in the repository, too? (We don't need to do that right away).

 
+# glpk's build system uses the output of grep
+unset GREP_OPTIONS
+

This change has been made in quite some places. Why wasn't GREP_OPTIONS being set a problem before?

diff --git a/sage b/sage
index 6597c54..46c69ba 100755
--- a/sage
+++ b/sage
@@ -124,11 +124,10 @@ fi
 export SAGE_ROOT
 
 # Run the actual Sage script
-if [ -x "$SAGE_ROOT/spkg/bin/sage" ]; then
-    "$SAGE_ROOT/spkg/bin/sage" "$@"
-elif [ -x "$SAGE_ROOT/local/bin/sage-sage" ]; then   # Support sage-4.x
-    export CUR=`pwd`
-    "$SAGE_ROOT/local/bin/sage-sage" "$@"
+if [ -x "$SAGE_ROOT/src/bin/sage" ]; then
+    "$SAGE_ROOT/src/bin/sage" "$@"
+elif [ -x "$SAGE_ROOT/local/bin/sage" ]; then # if in a striped binary
+    "$SAGE_ROOT/local/bin/sage" "$@"
 else
     echo >&2 "$0: no Sage installation found in \$SAGE_ROOT=$SAGE_ROOT"
     exit 1

It feels wrong to me to prefer the src/bin version of the sage script to the local/bin version. Why is this?

diff --git a/src/bin/sage b/src/bin/sage
index 5c886e3..3762d5d 100755
--- a/src/bin/sage
+++ b/src/bin/sage
<SNIP>
@@ -864,12 +825,8 @@ if [ "$1" = '-pkg_nc' -o "$1" = "--pkg_nc" ]; then
 fi
 
 if [ "$1" = '-sdist' -o "$1" = "--sdist" ]; then
-    if [ $# -ne 2 ]; then
-        echo >&2 "** MISSING VERSION NUMBER! **"
-        exit 2
-    fi
-    maybe_sage_location
-    exec sage-sdist $2 "$SAGE_ROOT"
+    shift
+    exec sage-sdist "$@"

Why doesn't this need sage location anymore?

@@ -904,25 +861,7 @@ fi

 if [ "$1" = '-upgrade' -o "$1" = "--upgrade" ]; then
     shift
-    cd "$SAGE_ROOT"
-
-    # People often move the Sage install right before doing the upgrade, so it's
-    # important to fix any path hardcoding issues first, or certain library
-    # links will fail.
-    sage-location || exit $?
-
-    # Run sage-upgrade twice since when installing sage-scripts and a
-    # running script changes, it gets confused and exits with an error.
-    # Running again (with the script replaced) then fixes the problem.
     sage-upgrade "$@"
-    if [ $? -eq 2 ]; then   # this exit code means the user elected not to do the upgrade at all.
-        exit 2
-    fi
-    echo "Double checking that all packages have been installed."
-    sage-upgrade "$@" || exit $?
-
-    # Check that Sage still works
-    sage-starts
     exit $?
 fi

Why don't we call sage-location anymore? and sage-starts?

diff --git a/src/bin/sage-bdist b/src/bin/sage-bdist
index a776900..c765017 100755
--- a/src/bin/sage-bdist
+++ b/src/bin/sage-bdist
<SNIP>
+# Clone Sage repository
+echo "Cloning Sage repository..."
+git clone "$SAGE_ROOT" "$TMP_DIR/$TARGET"
+( cd "$TMP_DIR/$TARGET" && git remote set-url origin git://github.com/sagemath/sage.git )

We need to set this to the trac server. Also, what branch does this clone? It should probably be the current one, because those are the binaries it copies. Is it?

diff --git a/src/bin/sage-download-file b/src/bin/sage-download-file
index cf7dce0..a3428df 100755
--- a/src/bin/sage-download-file
+++ b/src/bin/sage-download-file
@@ -33,7 +33,7 @@ if [ -z "$URL_GRABBER" ]; then
     if [ -n "$SAGE_LOCAL" ] && [ -x "$SAGE_LOCAL/bin/python" ]; then
         URL_GRABBER="$SAGE_LOCAL/bin/python"
     elif command -v curl &>/dev/null; then
-        URL_GRABBER="curl"
+        URL_GRABBER="curl --fail"
     elif command -v wget &>/dev/null; then
         URL_GRABBER="wget -nv -O-"
     # Pick Python last because we don't know which version it is,

This looks fine, but why wasn't it an issue before?

diff --git a/src/bin/sage-env b/src/bin/sage-env
index 6c2408a..5e5af1b 100644
--- a/src/bin/sage-env
+++ b/src/bin/sage-env
<SNIP>
 # Make Mercurial not use any user defaults (like enabling the pager extension)
 # This is needed in order to use mercurial in scripts.  When running hg
-# directly through "sage -hg", we will disable HGPLAIN in spkg/bin/sage.
+# directly through "sage -hg", we will disable HGPLAIN in src/bin/sage.
 # See Trac Ticket #12058
 HGPLAIN="yes" && export HGPLAIN

This can probably go.

+if [ -z "$SAGE_REPO" ]; then
+    SAGE_REPO="git://github.com/sagemath/sage.git"
+    export SAGE_REPO
+fi

This should be set to the trac server.

diff --git a/src/bin/sage-spkg b/src/bin/sage-spkg
index 20c56db..86c03c3 100755
--- a/src/bin/sage-spkg
+++ b/src/bin/sage-spkg

This file has had major modifications so it needs updated author credits. Also, the comment at the start still explains its old functionality.

 
+DOWNLOAD_ONLY=0
+if [ "$1" = '-d' ]; then
+    DOWNLOAD_ONLY=1
+    shift
+fi
+

I think this is an odd way to change the functionality. Maybe download-only should be in a script of itself.

diff --git a/src/bin/sage-sync-build.py b/src/bin/sage-sync-build.py
index 8e45386..b5d476a 100755
--- a/src/bin/sage-sync-build.py
+++ b/src/bin/sage-sync-build.py
<SNIP>

Why does sage-sync-build need so many changes? Naively, it would need only a few changes to some directory names.

diff --git a/src/bin/sage-upgrade b/src/bin/sage-upgrade
index 46c1935..55432d1 100755
--- a/src/bin/sage-upgrade
+++ b/src/bin/sage-upgrade
-./install
+if [ "$#" -gt 0 ]; then
+    BRANCH="$1"
+    shift
+else
+    BRANCH=$(git symbolic-ref HEAD) ||
+        BRANCH=$(git describe --exact-match --tags HEAD | \
+                 sed -e 's;^;refs/tags/;')
+    if [ -n "$BRANCH" ]; then
+        case $BRANCH in
+            refs/heads/master) BRANCH=master;;
+            refs/heads/release) BRANCH=release;;
+            refs/heads/beta) BRANCH=beta;;
+            refs/tags/*) BRANCH=release;;
+        esac
+    fi
+fi 

Maybe do a git fetch first? Also, should we add pushing to these branches in the repo to a script somewhere? Maybe to sage-sdist?

diff --git a/src/c_lib/SConstruct b/src/c_lib/SConstruct
index fd4719a..1813de8 100644
--- a/src/c_lib/SConstruct
+++ b/src/c_lib/SConstruct
-#Here we only copy the files over if we are on Cygwin.  Otherwise, the
-#library will be handled by the symlinks created in
-#$SAGE_ROOT/devel/sage/spkg-install
-if os.environ['UNAME'] == 'CYGWIN':
-    env.Alias("install", "$SAGE_LOCAL/lib")
-else:
-    env.Alias("install", [lib])
-
+env.Alias("install", "$SAGE_LOCAL")

Will this still work on Cygwin?

diff --git a/src/sage/misc/sagedoc.py b/src/sage/misc/sagedoc.py
index 9f0bbe3..12e8434 100644
--- a/src/sage/misc/sagedoc.py
+++ b/src/sage/misc/sagedoc.py
@@ -407,9 +407,9 @@ def format(s, embedded=False):
     EXAMPLES::
 
         sage: from sage.misc.sagedoc import format
-        sage: identity_matrix(2).rook_vector.__doc__[115:184]
+        sage: identity_matrix(2).rook_vector.__doc__[107:176]
         'Let `A` be a general `m` by `n`\n        (0,1)-matrix with `m \\le n`. '
-        sage: format(identity_matrix(2).rook_vector.__doc__[115:184])
+        sage: format(identity_matrix(2).rook_vector.__doc__[107:176])
         'Let A be a general m by n\n   (0,1)-matrix with m <= n.\n'
 
     If the first line of the string is 'nodetex', remove 'nodetex' but

why did this change? Is this related to the removal of whitespace? If so: okay.

diff --git a/src/sage/tests/cmdline.py b/src/sage/tests/cmdline.py
index 342c3ff..7e45f0f 100644
--- a/src/sage/tests/cmdline.py
+++ b/src/sage/tests/cmdline.py
@@ -208,15 +198,9 @@ def test_executable(args, input="", timeout=100.0, **kwds):
     Test ``sage --info [packages]``, unless this is a binary (bdist)
     distribution which doesn't ship spkgs::
 
-        sage: if os.path.isfile(os.path.join(SAGE_ROOT, 'spkg', 'standard', '.from_bdist')):
-        ...     out = "Found package sqlalchemy in spkg/standard/sqlalchemy-...spkg\n= SQLAlchemy =\n...\nSQLAlchemy is the Python SQL toolkit..."
-        ...     err = ''
-        ...     ret = 0
-        ... else:
-        ...     (out, err, ret) = test_executable(["sage", "--info", "sqlalchemy"])
-        ...
+        sage: out, err, ret = test_executable(["sage", "--info", "sqlalchemy"])
         sage: print out
-        Found package sqlalchemy in spkg/standard/sqlalchemy-...spkg
+        Using local scripts to install sqlalchemy-...
         = SQLAlchemy =
         ...
         SQLAlchemy is the Python SQL toolkit...

Do I understand correctly that we install sqlalchemy when running doctests? What is happening here?

diff --git a/src/spkg-delauto b/src/spkg-delauto
deleted file mode 100755
index 61d071d..0000000
--- a/src/spkg-delauto
+++ /dev/null

Why don't we need this anymore?

comment:13 in reply to: ↑ 12 Changed 6 years ago by ohanar

Replying to tkluck:

Maybe it makes sense for at least one more person to go through this diff.

Definitely. And hopefully someone who is already familiar with Sage's build system.

why do we clean more directories than before?

In order to make sure distclean still works as expected. Previously cleaning up everything wasn't important for this since distclean would just delete the whole devel directory.

What happened to this parallel_make.cfg file?

There is no other reference to it in the source code.

Do I understand correctly that, and is it okay that the micro_release code now only strips the binaries and does not remove build directories?

It seems like I didn't revert some cleanup I made to the Makefile in response to #14721.

diff --git a/build/gen_html b/build/gen_html
deleted file mode 100755
index 6dbad06..0000000
--- a/build/gen_html
+++ /dev/null

What did this script do? Why don't we need it anymore?

it is used to generate the webpages http://sagemath.org/packages/{standard,optional,experimental}, however the actual version of the script that is used in generating these patches disagrees from the version that is included in Sage. Since I was overhauling the build system (and this was in the directory related to the build system), I thought it wise to remove it to avoid any confusion.

Should we make sure that this does not break any optional/experimental packages? Personally, I'm fine with any package that installs anything into this data directory, which I'm guessing has been deprecated for some time now. (how long?)

This reminds me: will we, at some point, test the installation of the optional packages into the git framework? And, since we're already distributing them officially, couldn't we just as well put their metadata in the repository, too? (We don't need to do that right away).

We should at least make sure that all optional packages work. Basically all changes that might be needed should be fixable within the mercurial distribution. Since we will have to through the packages anyway, it made perfect sense to me to cleanup the whole SAGE_DATA thing. I've opened #14962 for this task.

This change has been made in quite some places. Why wasn't GREP_OPTIONS being set a problem before?

It was, however I didn't bother fixing it on the mercurial side because either you have to unset it in sage-env or you have to do it in a bunch of packages (although it just came to me that you could unset it in sage-spkg). Unsetting in sage-env is less than ideal because it means that commands like sage -grep don't respect GREP_OPTIONS, and changing a simple thing like this across a bunch of packages is incredibly painful in the current workflow.

It feels wrong to me to prefer the src/bin version of the sage script to the local/bin version. Why is this?

I've gone back and forth on this a few times. The reason why I ended up with this was because the latest version of the sage script will always be at src/bin, where as the version at local/bin will only be updated if you have rerun make.

diff --git a/src/bin/sage b/src/bin/sage
index 5c886e3..3762d5d 100755
--- a/src/bin/sage
+++ b/src/bin/sage
<SNIP>
@@ -864,12 +825,8 @@ if [ "$1" = '-pkg_nc' -o "$1" = "--pkg_nc" ]; then
 fi
 
 if [ "$1" = '-sdist' -o "$1" = "--sdist" ]; then
-    if [ $# -ne 2 ]; then
-        echo >&2 "** MISSING VERSION NUMBER! **"
-        exit 2
-    fi
-    maybe_sage_location
-    exec sage-sdist $2 "$SAGE_ROOT"
+    shift
+    exec sage-sdist "$@"

Why doesn't this need sage location anymore?

This was a merging error, sage-location should still be here.

Why don't we call sage-location anymore? and sage-starts?

I moved the sage-location call to sage-real-upgrade, and I think that sage-starts should probably be moved to sage-real-upgrade as well (which I think I meant to do, but it seems like I forgot).

We need to set this to the trac server. Also, what branch does this clone? It should probably be the current one, because those are the binaries it copies. Is it?

Without an argument git clone will clone HEAD.

diff --git a/src/bin/sage-download-file b/src/bin/sage-download-file
index cf7dce0..a3428df 100755
--- a/src/bin/sage-download-file
+++ b/src/bin/sage-download-file
@@ -33,7 +33,7 @@ if [ -z "$URL_GRABBER" ]; then
     if [ -n "$SAGE_LOCAL" ] && [ -x "$SAGE_LOCAL/bin/python" ]; then
         URL_GRABBER="$SAGE_LOCAL/bin/python"
     elif command -v curl &>/dev/null; then
-        URL_GRABBER="curl"
+        URL_GRABBER="curl --fail"
     elif command -v wget &>/dev/null; then
         URL_GRABBER="wget -nv -O-"
     # Pick Python last because we don't know which version it is,

This looks fine, but why wasn't it an issue before?

It was, but it is more of an issue now because sage-spkg now tries a few urls for each package, which requires sage-download-file to bail if the server returns an error.

diff --git a/src/bin/sage-sync-build.py b/src/bin/sage-sync-build.py
index 8e45386..b5d476a 100755
--- a/src/bin/sage-sync-build.py
+++ b/src/bin/sage-sync-build.py
<SNIP>

Why does sage-sync-build need so many changes? Naively, it would need only a few changes to some directory names.

Currently we are symlinking stuff into SAGE_LOCAL from the devel directory (this is to make cloning fast), however this is no longer happening in the git repository. This caused some silly breaking with the sage-sync-build, which was in need of some heavy revision anyway. There is now another ticket out there (don't remember which one) dedicated to fixing + updating this script.

diff --git a/src/bin/sage-upgrade b/src/bin/sage-upgrade
index 46c1935..55432d1 100755
--- a/src/bin/sage-upgrade
+++ b/src/bin/sage-upgrade
-./install
+if [ "$#" -gt 0 ]; then
+    BRANCH="$1"
+    shift
+else
+    BRANCH=$(git symbolic-ref HEAD) ||
+        BRANCH=$(git describe --exact-match --tags HEAD | \
+                 sed -e 's;^;refs/tags/;')
+    if [ -n "$BRANCH" ]; then
+        case $BRANCH in
+            refs/heads/master) BRANCH=master;;
+            refs/heads/release) BRANCH=release;;
+            refs/heads/beta) BRANCH=beta;;
+            refs/tags/*) BRANCH=release;;
+        esac
+    fi
+fi 

Maybe do a git fetch first?

Good idea.

Also, should we add pushing to these branches in the repo to a script somewhere? Maybe to sage-sdist?

Pushing should only be done by the release manager. I think the best place for such stuff would be as part of the development scripts, although I think adding the release manager portion to these scripts should be lower priority than the general developer portion of these scripts.

diff --git a/src/c_lib/SConstruct b/src/c_lib/SConstruct
index fd4719a..1813de8 100644
--- a/src/c_lib/SConstruct
+++ b/src/c_lib/SConstruct
-#Here we only copy the files over if we are on Cygwin.  Otherwise, the
-#library will be handled by the symlinks created in
-#$SAGE_ROOT/devel/sage/spkg-install
-if os.environ['UNAME'] == 'CYGWIN':
-    env.Alias("install", "$SAGE_LOCAL/lib")
-else:
-    env.Alias("install", [lib])
-
+env.Alias("install", "$SAGE_LOCAL")

Will this still work on Cygwin?

I think so, although we really should have someone on Cygwin test. As I mentioned, the symlinks that the comment refers to are no more.

diff --git a/src/sage/tests/cmdline.py b/src/sage/tests/cmdline.py
index 342c3ff..7e45f0f 100644
--- a/src/sage/tests/cmdline.py
+++ b/src/sage/tests/cmdline.py
@@ -208,15 +198,9 @@ def test_executable(args, input="", timeout=100.0, **kwds):
     Test ``sage --info [packages]``, unless this is a binary (bdist)
     distribution which doesn't ship spkgs::
 
-        sage: if os.path.isfile(os.path.join(SAGE_ROOT, 'spkg', 'standard', '.from_bdist')):
-        ...     out = "Found package sqlalchemy in spkg/standard/sqlalchemy-...spkg\n= SQLAlchemy =\n...\nSQLAlchemy is the Python SQL toolkit..."
-        ...     err = ''
-        ...     ret = 0
-        ... else:
-        ...     (out, err, ret) = test_executable(["sage", "--info", "sqlalchemy"])
-        ...
+        sage: out, err, ret = test_executable(["sage", "--info", "sqlalchemy"])
         sage: print out
-        Found package sqlalchemy in spkg/standard/sqlalchemy-...spkg
+        Using local scripts to install sqlalchemy-...
         = SQLAlchemy =
         ...
         SQLAlchemy is the Python SQL toolkit...

Do I understand correctly that we install sqlalchemy when running doctests? What is happening here?

This can be changed in sage-spkg. That message just prints when it realizes it has the package metadata locally.

diff --git a/src/spkg-delauto b/src/spkg-delauto
deleted file mode 100755
index 61d071d..0000000
--- a/src/spkg-delauto
+++ /dev/null

Why don't we need this anymore?

no other reference in the source code (except for MANIFEST.in, which should be updated)

comment:14 Changed 6 years ago by tkluck

Mind if I start making some of the (simpler) changes?

Currently we are symlinking stuff into SAGE_LOCAL from the devel directory (this is to make cloning fast), however this is no longer happening in the git repository. This caused some silly breaking with the sage-sync-build, which was in need of some heavy revision anyway. There is now another ticket out there (don't remember which one) dedicated to fixing + updating this script.

I couldn't find the ticket. Do you mean that your edits basically fix the ticket? Or do you mean that the version of sage-sync-build in your build_system branch is still in need of some heavy revision?

It feels wrong to me to prefer the src/bin version of the sage script to the local/bin version. Why is this?

I've gone back and forth on this a few times. The reason why I ended up with this was because the latest version of the sage script will always be at src/bin, where as the version at local/bin will only be updated if you have rerun make.

I agree that this is the behavior, but I'm not sure whether that's what we want. After switching branches, you could end up in the situation where the sage script from one branch is using SAGE_LOCAL files from another branch. Imagine that a branch adds both a package imagemagick and a command sage -convert and it seems much more consistent to me to only run the new sage script after make has been called.

But I guess that I don't mind much; whoever is in a situation where the difference affects them, is probably doing something with the internal workings of sage anyway, and should be able to deal with it.

comment:15 follow-up: Changed 6 years ago by felixs

I think it makes sense to move build/pkgs to pkgs. My current work on top of build_system uses pkgs for the package sources and build is used to build packages. Would it be possible to do this move in build_system?

comment:16 in reply to: ↑ 15 Changed 6 years ago by ohanar

Ok, I think I addressed Timo's comments, with the following exceptions:

HG configuration in sage-env: I don't see a good reason for getting rid of these in the short term, we may not be using hg anymore, but it could still be helpful for the transition period while repositories there are still HG repositories for optional packages.

GREP_OPTIONS: I'm not going to fix this now, instead leave it to a later date (unless I encounter merge conflicts, which so far I haven't).

Replying to felixs:

I think it makes sense to move build/pkgs to pkgs. My current work on top of build_system uses pkgs for the package sources and build is used to build packages. Would it be possible to do this move in build_system?

I would rather not, I don't see a huge need for change at this moment and currently everything works. We can always change this later down the road, but for now I would like to get this ticket finished.


One other thing is that some functionality (especially sdist and bdist) might be broken until I merge the git spkg, which I will do in this ticket. However, there is still plenty of other code that needs review so I'm going to mark this to needs review for the moment.

comment:17 Changed 6 years ago by ohanar

  • Status changed from needs_info to needs_review

comment:18 Changed 6 years ago by ohanar

ok, merged in the git spkg, so all should be good to go

comment:19 Changed 6 years ago by git

  • Commit set to c7903c067f0fa949bcd09e6e60c6c306dee26ab4

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

comment:20 Changed 6 years ago by jdemeyer

Some comments (I will probably have several, so I post them one by one):

In top-level Makefile in lib-clean, replace

        rm -rf src/sage/libs/pari/gen.h
        rm -rf src/sage/modular/arithgroup/farey_symbol.h
        rm -rf src/sage/rings/real_mpfi.h
        rm -rf src/sage/symbolic/pynac.h

by

        rm -f src/sage/modular/arithgroup/farey_symbol.h
        rm -f src/sage/rings/real_mpfi.h
        rm -f src/sage/symbolic/pynac.h

(the removal of gen.h is #15124).

Actually, I would prefer a multi-level Makefile (this is the preferred way for automake). That is, move lib-clean to a file src/Makefile and have the top-level Makefile contain:

lib-clean:
    cd src && $(MAKE) clean
Last edited 6 years ago by jdemeyer (previous) (diff)

comment:21 Changed 6 years ago by jdemeyer

SCRIPT_SOURCES and the likes in build/Makefile (generated by build/install): why use absolute filenames instead of environment variables like $(SAGE_LOCAL)? This might break relocation.

comment:22 Changed 6 years ago by jdemeyer

build/Makefile: what does @mkdir -p "$(@D)" do? Add a comment explaining this.

comment:23 Changed 6 years ago by jdemeyer

src/bin/sage-spkg: replace

    RESULT=`GREP_OPTIONS= grep "^$param\ *="  $file| sed "s/^$param\ *=\ *//"`
    echo $RESULT

by

    sed -n "s/^${param} *= *//p" $file
Last edited 6 years ago by jdemeyer (previous) (diff)

comment:24 follow-up: Changed 6 years ago by jdemeyer

Concerning checksums: why not add Python support? For the buildbots, I have this script to verify checksums of Sage tarballs:

#!/usr/bin/env python

import hashlib
import sys

H = hashlib.sha256()

while True:
    d = sys.stdin.read(4096)
    H.update(d)
    if not len(d):
        break

print H.hexdigest()

You could have a variant of this be called shasum.py and then run python shasum.py if Python exists (command -v python).

comment:25 Changed 6 years ago by jdemeyer

In shell scripts, all instances of [ A == B ] should be replaced by [ A = B ].

comment:26 Changed 6 years ago by jdemeyer

In src/bin/sage-spkg, what is the purpose of USE_LOCAL_SCRIPTS? It is difficult for me to understand, perhaps some comment should be added.

comment:27 follow-up: Changed 6 years ago by jdemeyer

I dislike local/var/tmp/sage/build. One often needs to go into this directory to debug stuff, could you not use an easier directory instead?

comment:28 in reply to: ↑ 27 Changed 6 years ago by ohanar

Replying to jdemeyer:

I dislike local/var/tmp/sage/build. One often needs to go into this directory to debug stuff, could you not use an easier directory instead?

Sure, any suggestions (I would like to keep it under local somewhere, as it in the longterm consolidates the directories that would need to be cleaned).

Last edited 6 years ago by ohanar (previous) (diff)

comment:29 Changed 6 years ago by git

  • Commit changed from c7903c067f0fa949bcd09e6e60c6c306dee26ab4 to 1a65c42fcce72b927d76fe7dfc9d6779c80c0cef

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

comment:30 Changed 6 years ago by git

  • Commit changed from 1a65c42fcce72b927d76fe7dfc9d6779c80c0cef to 50e57d75d53601bc073a855ce995d7aa92506a92

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

comment:31 Changed 6 years ago by roed

  • Status changed from needs_review to needs_info

What's the status of this ticket? Andrew, are you going to be able to address Jeroen's concerns soon?

comment:32 Changed 6 years ago by git

  • Commit changed from 50e57d75d53601bc073a855ce995d7aa92506a92 to c29a7554ab4cfd9f0d17a8a242558db5b820b12a

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

comment:33 in reply to: ↑ 24 Changed 6 years ago by ohanar

  • Commit changed from c29a7554ab4cfd9f0d17a8a242558db5b820b12a to 50e57d75d53601bc073a855ce995d7aa92506a92

Replying to jdemeyer:

Concerning checksums: why not add Python support? For the buildbots, I have this script to verify checksums of Sage tarballs:

You could have a variant of this be called shasum.py and then run python shasum.py if Python exists (command -v python).

I don't quite understand why we should duplicate this functionality in python, since we already have a working version in shell.

comment:34 Changed 6 years ago by git

  • Commit changed from 50e57d75d53601bc073a855ce995d7aa92506a92 to 86a61958f775df75bfabb035ffeeaffd69ffdec6

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

comment:35 Changed 6 years ago by git

  • Commit changed from 86a61958f775df75bfabb035ffeeaffd69ffdec6 to bf481816e69f86973d24a18e7df25852b69a2a8a

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

[changeset:bf48181]Trac #14908: Two small issues with make micro_release
[changeset:0628ee3]Fix two small issues with make micro_release

comment:36 Changed 6 years ago by vbraun

  • Status changed from needs_info to needs_review

Why is this needs info? Because we want to know its status? Isn't that a bit self-referential?

I think all of Jeroen's comments were addressed except the build dir (though I agree that keeping it under local is a good idea, and the old build dir wasn't particularly short either) and using Python to compute the hash (better solution than calling external program, but might depend on whether Python build hashlib correctly which IIRC sometimes causes trouble). In any case both can be easily changed later on, so I don't think we necessarily have to act on them now.

comment:37 Changed 6 years ago by git

  • Commit changed from bf481816e69f86973d24a18e7df25852b69a2a8a to 5677976a6fdd6f7a1c1f5e634868accd6be0eeac

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

[changeset:5677976]Trac #15120: Fix doctest doctests in git layout.
[changeset:e11727c]revert the change to .gitignore
[changeset:6912669]Fix --new option for doctests.
[changeset:98d0840]Fix doctest doctest due to directory restructure.

comment:38 Changed 6 years ago by git

  • Commit changed from 5677976a6fdd6f7a1c1f5e634868accd6be0eeac to 56d80d07ad270272a64e8c71fd7a9bc632f37ba6

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

[changeset:56d80d0][FIXUP] 5.12.rc0: update git specific files
[changeset:4aa23b7]Merge branch 'master' into build_system
[changeset:df16d32]Merge 'sage-5.12.rc0' into master
[changeset:45a79e8]5.12.rc0
[changeset:0f00389]5.12.rc0
[changeset:a77c210]Trac #13770: Add upstream pull request 215 for multivariate factorisation
[changeset:18be736]Trac #13770: bug in multivariate factorization over prime fields
[changeset:59c8447]Trac #15194: added imports and doctests for MIPSolver exceptions
[changeset:49dc953]Trac #14776: implements StrongTableau? and StrongTableaux? classes
[changeset:a854c61]Trac #15204: fix gap cyclotomics to sage

comment:39 Changed 6 years ago by git

  • Commit changed from 56d80d07ad270272a64e8c71fd7a9bc632f37ba6 to ca97e70c81f671ba4413ecb1530331d91560c46f

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

[changeset:ca97e70][FIXUP] 5.12.rc1: update git specific files
[changeset:9cf9b36]Merge branch 'master' into build_system
[changeset:e061dd3]Merge 'sage-5.12.rc1' into master
[changeset:864c388]5.12.rc1
[changeset:dab4c3f]5.12.rc1
[changeset:9c2f74f]Trac #15252: Doctest tolerance
[changeset:07f335d]Trac #13948: fix various configure tests which break with Clang
[changeset:463ace3]Trac #15189: fixed recursion in _equivalence method used by is_field_isomorphic
[changeset:b856810]Trac #15193: add method Factorization._pari_(), using itertools.chain
[changeset:05fdb53]Trac #13948: Document that Sage can now be bootstrapped with Clang

comment:40 Changed 6 years ago by git

  • Commit changed from ca97e70c81f671ba4413ecb1530331d91560c46f to 60d5a0630a478a7778110581fc46ef4b9c6dd8ab

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

comment:41 Changed 6 years ago by jhpalmieri

I wonder if there should be a symbolic link from "doc" at the top level pointing to src/doc/output/html/en. Not exactly a new directory layout, but in the spirit of this ticket: beginning users might be comforted to find the documentation in an obvious place. This link should probably created at the end of the build process, after building the docs (part of the doc target in Makefile).

This could also be done on a separate ticket, of course.

comment:42 Changed 6 years ago by git

  • Commit changed from 60d5a0630a478a7778110581fc46ef4b9c6dd8ab to ec4ed117d844ca2ce24f16e01e5e61e5683ef4ba

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

[changeset:ec4ed11][FIXUP] 5.13.beta1: update git specific files
[changeset:a23df3e]Merge branch 'upstream' into build_system
[changeset:b7dc123]Merge 'sage-5.13.beta1'
[changeset:8693e80]5.13.beta1
[changeset:755697a]5.13.beta1
[changeset:e3808e5]Trac #13726: Semimonomial transformation group
[changeset:d182ea9]Trac #15279: init of RootSystem? corrected
[changeset:f5adfd5]Solaris / ZFS fix
[changeset:d13cee2]Trac #15270: Do not give up if the upstream shared library build fails
[changeset:d25cd84]Trac #10589: Updating the message for the fixdoctests option printed by --advanced

comment:43 Changed 6 years ago by git

  • Commit changed from ec4ed117d844ca2ce24f16e01e5e61e5683ef4ba to 952f7e3d12a82a40e6c0365f98f0e3984b4308ca

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

comment:44 Changed 6 years ago by vbraun

The docs en/reference/misc/index.rst still reference the deleted sage/misc/hg, leading to a docbuild error. I think this branch would be the right place to fix the fallout.

As for the doc symlink, we should probably disentangle the src dir from build output altogether. Documentation should probably go into local/share/sage/doc. But first we really should finalize the git transition.

comment:45 Changed 6 years ago by git

  • Commit changed from 952f7e3d12a82a40e6c0365f98f0e3984b4308ca to 874ba344029a1baae5d9ab8ba6554f5a0d0126a7

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

comment:46 Changed 6 years ago by git

  • Commit changed from 874ba344029a1baae5d9ab8ba6554f5a0d0126a7 to f5796f2d3508aa39ef578121d0796d1bf3fecfbe

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

f5796f2remove reference to sage/misc/hg from docbuild

comment:47 Changed 6 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

Looks good to me!

comment:48 Changed 6 years ago by git

  • Commit changed from f5796f2d3508aa39ef578121d0796d1bf3fecfbe to 1b1d23620cb9e912c21afe6ec75aba322d6b2dd5
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

comment:49 Changed 6 years ago by vbraun

  • Status changed from needs_review to positive_review

For the record, this ticket is positively reviewed. Even if Andrew keeps merging the sage-hg branch until sage-5.13 is finished.

comment:50 follow-up: Changed 6 years ago by git

  • Commit changed from 1b1d23620cb9e912c21afe6ec75aba322d6b2dd5 to 47cb70ddd7f01952996ba3d404d5d4c12a4dae7e
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

comment:51 in reply to: ↑ 50 Changed 6 years ago by mmezzarobba

Hi,

The merge of 5.13-beta4 broke make build for me, because http://www.sagemath.org/packages/upstream/r/r-3.0.2.tar is missing. See also http://trac.sagemath.org/ticket/14706#comment:119.

comment:52 Changed 6 years ago by git

  • Commit changed from 47cb70ddd7f01952996ba3d404d5d4c12a4dae7e to 33dec52b43f2fcc22318129da442e5bdb8a24b28

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

comment:53 Changed 6 years ago by git

  • Commit changed from 33dec52b43f2fcc22318129da442e5bdb8a24b28 to cd237e726293c48288016aa2a9cc80aa75a3bd20

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

comment:54 Changed 6 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:55 Changed 6 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.