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

dev docs: Inclusion Procedure for New and Updated Packages #16048

Closed
rwst opened this issue Apr 2, 2014 · 34 comments
Closed

dev docs: Inclusion Procedure for New and Updated Packages #16048

rwst opened this issue Apr 2, 2014 · 34 comments

Comments

@rwst
Copy link

rwst commented Apr 2, 2014

As discussed in

https://groups.google.com/forum/?hl=en#!topic/sage-devel/jopuoWO1Fvk

clear documentation on the procedure is missing.

Component: documentation

Author: Ralf Stephan, Vincent Delecroix

Branch: ecba6b6

Reviewer: R. Andrew Ohana, Jeroen Demeyer, Vincent Delecroix, Ralf Stephan

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

@rwst rwst added this to the sage-6.2 milestone Apr 2, 2014
@rwst
Copy link
Author

rwst commented Apr 2, 2014

Branch: public/spkg-devdoc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 2, 2014

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

bf1291aTrac 16048: Inclusion Procedure for New and Updated Packages

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 2, 2014

Commit: bf1291a

@ohanar
Copy link
Member

ohanar commented Apr 2, 2014

comment:4

There is a bit of redundancy in your first paragraph and the 3rd about a package requiring a year before it could be considered for a standard package. Additionally, experimental packages aren't really ever eligible to become standard packages since standard packages need to support all of Sage's supported platforms (and most things that fall into the experimental category are things that break on various platforms).

Also, I don't know if it is mentioned in any of the surrounding documentation, but standard packages need to be GPLv3+ compatible, so including that somewhere would be good.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 3, 2014

Changed commit from bf1291a to 5e72e74

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 3, 2014

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

5e72e7416048: minor adaptations

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 12, 2014

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

a5f8a33Merge branch 'develop' into t/16048/public/spkg-devdoc
a05ec4616048: add paragraph about ready to use spkgs

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 12, 2014

Changed commit from 5e72e74 to a05ec46

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 13, 2014

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

77ce76e16048: fixes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 13, 2014

Changed commit from a05ec46 to 77ce76e

@vbraun
Copy link
Member

vbraun commented Apr 14, 2014

comment:8

Does having both a SPKG and a build/pkg/foo directory work? Which spkg-install script is being used? I don't even know if this works, but it shouldn't. You can either have a legacy spkg or make a git branch with an upstream tarball (gz/bz2).

@rwst
Copy link
Author

rwst commented Apr 15, 2014

Changed branch from public/spkg-devdoc to public/spkg-devdoc-1

@rwst
Copy link
Author

rwst commented Apr 15, 2014

Changed commit from 77ce76e to 469f971

@rwst
Copy link
Author

rwst commented Apr 15, 2014

comment:9

OK, but what to do if the supporting files in the published SPKG are defect, see #15813?


New commits:

d993164Trac 16048: Inclusion Procedure for New and Updated Packages
1db8cd716048: minor adaptations
b23109216048: add paragraph about ready to use spkgs
bec1ced16048: fixes
469f97116048: improve previous change

@jdemeyer
Copy link

comment:10

I really don't understand what you're trying to say with

A special case where no patch would be necessary is when an author
provides an already fine SPKG on the net which includes all files
needed for ``SAGE_ROOT/build/pkgs/foo`` and the source in its ``src/``
subdirectory. Here it suffices to put the web link to the package
into the ticket. Alternatively, you would have to ask the author to
make a tarball of the ``src/`` subdirectory, and upload a branch with
``SAGE_ROOT/build/pkgs/foo``.

The first part of the patch is good for me.

@videlec
Copy link
Contributor

videlec commented Apr 16, 2014

comment:11

Hi,

  1. It would be nice to start with a section explaining how the installation procedure works ("how does this work" or "the big picture") and make appropriate pointers to the install scripts. Part of it is already explained in "Directory structure".

  2. There is a lot of intersection between your new section "Procedure for ..." and the "Prerequisites". It would make more sense to have only one.

  3. About "Testing". Firstly there is a typo !``SAGE_ROOT/upstream/` should be closed with two backquotes. Secondly, the name of the section is not very appropriate as it intersects with "Self-test". It would also be better if the content of this section appear inside "how this work" mentioned in 0).

Vincent

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 17, 2014

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

42e05b116048: enhancements; typos fixed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 17, 2014

Changed commit from 469f971 to 42e05b1

@rwst
Copy link
Author

rwst commented Apr 17, 2014

comment:13

Thanks to all reviewers so far.

Replying to @videlec:

  1. There is a lot of intersection between your new section "Procedure for ..." and the "Prerequisites". It would make more sense to have only one.

I will take the liberty to disagree here, for educational reasons.

Everything else was fixed. Anything amiss in this section?

@rwst
Copy link
Author

rwst commented Apr 17, 2014

Reviewer: R. Andrew Ohanar, Jeroen Demeyer, Vincent Delecroix

@videlec
Copy link
Contributor

videlec commented Apr 17, 2014

comment:14

I am very sorry to be very picky but I definitely think that the documentation is an important piece of a software. Please for each of my comment, say explicitly if you agree or not (you did it for my 1) before but you mostly eluded the other two). I am happy to make the changes you accept by myself if you prefer.

section "Inclusion Procedure for New and Updated Packages"

As it is the case in all other sections, it would be nice if you add a line break after the title.

For the politics about tickets, I thought that:

  • experimental = does not build on all platforms officially supported => never be optional
  • optional = does build on all platforms => can possibly become standard after some time

The sentence "In short, packages consist of a link to the original tarball with additional
code under !SAGE_ROOT/build/pkgs!" is not meaningful at this stage of the document. It is not clear what is a "package" nor what is an "original tarball". I would simply remove the sentence as this section is purely about "how do you submit" and not "how do you make your own package". Moreover, I would would better put this section near the end of the document because before submitting a package you need to build one ;-).

section "Directory structure"

In the sentence "to package it in Sage" you must uppercase the "t". At the very same place I suggest to put a comment in parenthesis "... distribute a tarball !foo-1.3.tar.gz! (that will be automatically placed in !SAGE_ROOT/upstream! during the installation process)" or something similar.

section "Manual package build and installation"

I think that the shell command

$ SAGE_CHECK=yes sage -f package_name

will not do what you expect. You might either put a column between the SAGE_CHECK=yes and the sage -f package_name or put it in two lines.

There are at least 5 important missing things:

  • sage -i PACKAGE_NAME is the standard command to install a package
  • if a package is already installed, we can perform the tests only with sage -i -c PACKAGE_NAME (what the -c option does is that it set SAGE_CHECK to yes + some extra mystery about SAGE_CHECK_PACKAGES)
  • to force the reinstallation of a package we can do sage -i -f PACKAGE_NAME (and sage -f PACKAGE_NAME is a shortcut for it)
  • the install script is in $SAGE_ROOT/local/bin/install (and Sage reinvented the wheel here!!). It is relatively well documented and is worth to points its existence.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2014

Changed commit from 42e05b1 to b0a7ee5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2014

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

b0a7ee516048: more changes

@rwst
Copy link
Author

rwst commented Apr 18, 2014

comment:16

Thanks for your review.

Replying to @videlec:

I am very sorry to be very picky

Don't.

As it is the case in all other sections, it would be nice if you add a line break after the title.

Done.

  • experimental = does not build on all platforms officially supported => never be optional

Yes, done.

The sentence "In short, packages consist of a link to the original tarball with additional
code under !SAGE_ROOT/build/pkgs!" is not meaningful at this stage of the document. It is not clear what is a "package" nor what is an "original tarball". I would simply remove the sentence as this section is purely about "how do you submit" and not "how do you make your own package".

Done.

Moreover, I would would better put this section near the end of the document because before submitting a package you need to build one ;-).

I was not sure how to do this.

In the sentence "to package it in Sage" you must uppercase the "t". At the very same place I suggest to put a comment in parenthesis "... distribute a tarball !foo-1.3.tar.gz! (that will be automatically placed in !SAGE_ROOT/upstream! during the installation process)" or something similar.

Both done.

$ SAGE_CHECK=yes sage -f package_name

will not do what you expect.

I replaced it with sage -i -c PACKAGE_NAME.

There are at least 5 important missing things:

I was unsure how to include this. Please feel free to add it and the other issue above yourself.

@videlec
Copy link
Contributor

videlec commented Apr 22, 2014

comment:17

Hi Ralf,

I added few sentences in the introduction about the install script and the organization of the document. I also reorganize the sections (not changing them).

In my second commit I just set the line width to 72.

Tell me if you think it is better (as I am not a native english speaker there might be some mistakes). Otherwise we can just go back to the previous branch public/spkg-devdoc-1 which is just two commits before.

Best
Vincent


New commits:

56de113reorganisation + mention of install scripts in packaging.rst
ecba6b6set line width to 72 characters in packaging.rst

@videlec
Copy link
Contributor

videlec commented Apr 22, 2014

Changed branch from public/spkg-devdoc-1 to public/spkg-devdoc-2

@videlec
Copy link
Contributor

videlec commented Apr 22, 2014

Changed commit from b0a7ee5 to ecba6b6

@rwst
Copy link
Author

rwst commented Apr 22, 2014

comment:18

I think this is fine and I like it better. If you don't mind I'll set the ticket fields accordingly.

@rwst
Copy link
Author

rwst commented Apr 22, 2014

Changed author from Ralf Stephan to Ralf Stephan, Vincent Delecroix

@rwst
Copy link
Author

rwst commented Apr 22, 2014

Changed reviewer from R. Andrew Ohanar, Jeroen Demeyer, Vincent Delecroix to R. Andrew Ohanar, Jeroen Demeyer, Vincent Delecroix, Ralf Stephan

@videlec
Copy link
Contributor

videlec commented Apr 22, 2014

comment:19

Replying to @rwst:

I think this is fine and I like it better. If you don't mind I'll set the ticket fields accordingly.

Prefect, thanks!

@vbraun
Copy link
Member

vbraun commented Apr 22, 2014

Changed branch from public/spkg-devdoc-2 to ecba6b6

@kcrisman
Copy link
Member

Changed reviewer from R. Andrew Ohanar, Jeroen Demeyer, Vincent Delecroix, Ralf Stephan to R. Andrew Ohana, Jeroen Demeyer, Vincent Delecroix, Ralf Stephan

@kcrisman
Copy link
Member

Changed commit from ecba6b6 to none

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

6 participants