sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.45k stars 480 forks source link

Update section "Sage Development Process" in Developer's Guide #29784

Closed mkoeppe closed 2 years ago

mkoeppe commented 4 years ago

(from #29733)

Let the documentation, in particular the developer walk-through at

better reflect recent changes to the readme.

Mention that make takes very long.

Add discussion of make build.

Update Git instructions as discussed in

CC: @tobiasdiez @dimpase @jhpalmieri @slel @yuan-zhou @DavidAyotte @nbruin @egourgoulhon

Component: documentation

Author: Matthias Koeppe

Branch/Commit: d04d123

Reviewer: Eric Gourgoulhon, Tobias Diez, Samuel Lelièvre

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

tobiasdiez commented 4 years ago
comment:1

It might be also helpful to add the steps from #25206 comment:63 to the documentation.

mkoeppe commented 4 years ago
comment:2

Indeed. Are you planning to work on this?

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -2,4 +2,6 @@

 the documentation at ​https://doc.sagemath.org/html/en/developer/walk_through.html is not really reflecting some of the recent changes to the readme

+Also the reviewer's checklist https://doc.sagemath.org/html/en/developer/reviewer_checklist.html
+should be updated using material from https://wiki.sagemath.org/ReviewChecklist  and that wiki page replaced by a link to the manual.
mkoeppe commented 3 years ago

Description changed:

--- 
+++ 
@@ -5,3 +5,11 @@
 Also the reviewer's checklist https://doc.sagemath.org/html/en/developer/reviewer_checklist.html
 should be updated using material from https://wiki.sagemath.org/ReviewChecklist  and that wiki page replaced by a link to the manual.

+`make` takes very long. Could add discussion of `make build`
+
+It could also explain when make ends in  Error building Sage, what to do next
+
+
+
+
+
mkoeppe commented 3 years ago
comment:7

Moving to 9.4, as 9.3 has been released.

mkoeppe commented 3 years ago

Description changed:

--- 
+++ 
@@ -9,6 +9,7 @@

 It could also explain when make ends in  Error building Sage, what to do next

+Also git instructions need updating as discussed in https://groups.google.com/g/sage-devel/c/e9Nt_E4glkM/m/Lpmz_va_BAAJ
mkoeppe commented 3 years ago

Description changed:

--- 
+++ 
@@ -11,6 +11,6 @@

 Also git instructions need updating as discussed in https://groups.google.com/g/sage-devel/c/e9Nt_E4glkM/m/Lpmz_va_BAAJ

+Also should explain `configure --enable-editable` (https://wiki.sagemath.org/ReleaseTours/sage-9.3#Editable_.28.22in-place.22.2C_.22develop.22.29_installs_of_the_Sage_library)

-
mkoeppe commented 2 years ago

Branch: u/mkoeppe/update_sectionsage_development_processin_developer_s_guide

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

831e3a7src/doc/en/developer/walk_through.rst: Remove duplicated (incomplete) build instructions
4c1d1f4src/doc/en/developer/walk_through.rst: Switch from 'make' to 'make build' for rebuilds
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Commit: 4c1d1f4

mkoeppe commented 2 years ago

Author: Matthias Koeppe

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

7f00cbfREADME.md: Fix typo
5200907README.md: Remove link to 'Sage virtual appliance'
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 4c1d1f4 to 5200907

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

ff4d378README.md, src/doc/en/installation/source.rst: Reduce duplication of installation steps
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 5200907 to ff4d378

egourgoulhon commented 2 years ago
comment:17

Thanks for the update!

Maybe in README.md, replace

12. Type `make`.  That's it! Everything is automatic and
   non-interactive; but it will take a few hours (on a recent
   computer).

by something like

12. Type `make`.  That's it! Everything is automatic and
   non-interactive. NB: you can shorten the build time by typing instead 
   `MAKE='make -jNUM' make`, thereby telling the `make` program to run `NUM` jobs 
   in parallel. A recommended value for `NUM` is twice the number of cores of 
   your CPU. With `NUM` = 8, building Sage takes less than one hour on a modern 
   computer. 
dimpase commented 2 years ago
comment:18

With NUM = 8, building Sage takes less than one hour on a modern

computer. `

This is only true if many packages come from the system. Of course building gcc takes more time...

slel commented 2 years ago
comment:19

How about make -s V=0 for more zen? The log files will have all the info.

Though it also makes sense to give the simplest instructions in this guide.

mkoeppe commented 2 years ago
comment:20

Replying to @slel:

How about make -s V=0 for more zen?

step 9 already recommends export V=0.

mkoeppe commented 2 years ago
comment:21

Replying to @egourgoulhon:

Maybe in README.md, [...] something like

12. Type `make`.  That's it! Everything is automatic and
   non-interactive. NB: you can shorten the build time by typing instead 
   `MAKE='make -jNUM' make`, thereby telling the `make` program to run `NUM` jobs 
   in parallel. A recommended value for `NUM` is twice the number of cores of 
   your CPU. With `NUM` = 8, building Sage takes less than one hour on a modern 
   computer. 

I've made a change that instead refers back to previous steps

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

ef50064README.md: Update estimate for build time
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from ff4d378 to ef50064

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from ef50064 to 3949999

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

3949999src/doc/en/installation/source.rst: Add [GitHub](../wiki/GitHub) link for file README.md
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

ad38384src/doc/en/installation/source.rst: Update timestamp
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 3949999 to ad38384

mkoeppe commented 2 years ago
comment:25

Many more improvements could be done in the "installation from source" chapter, but I'll stop here for this ticket.

Let's get this in please

egourgoulhon commented 2 years ago

Attachment: README_indent.md.gz

fix indentation

egourgoulhon commented 2 years ago
comment:26

Replying to @mkoeppe:

Replying to @egourgoulhon:

Maybe in README.md, [...] something like

12. Type `make`.  That's it! Everything is automatic and
   non-interactive. NB: you can shorten the build time by typing instead 
   `MAKE='make -jNUM' make`, thereby telling the `make` program to run `NUM` jobs 
   in parallel. A recommended value for `NUM` is twice the number of cores of 
   your CPU. With `NUM` = 8, building Sage takes less than one hour on a modern 
   computer. 

I've made a change that instead refers back to previous steps

Much better indeed.

There are some indentation issues in README.md (sublist should be indented with 4 char tab), which result in bad output with some markdown viewers such as retext. This is corrected in the attached file

egourgoulhon commented 2 years ago
comment:27

Otherwise, LGTM.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

af70d2aREADME.md: Fix indentation
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from ad38384 to af70d2a

egourgoulhon commented 2 years ago
comment:29

Thanks!

egourgoulhon commented 2 years ago

Reviewer: Eric Gourgoulhon

mkoeppe commented 2 years ago
comment:30

Thank you!

tobiasdiez commented 2 years ago
comment:31

Concerning the change

Windows (WSL) Install the package using the Linux distribution's package manager. Do not attempt to use native Windows installations of git.

I don't think one strictly needs to exclude the Windows git. I had no problems so far using it. Based on other discussions on similar points, I would propose to formulate it as a strong recommendation to use the Linux git, but the native Windows way should be described as well.

tobiasdiez commented 2 years ago
comment:32

Concerning

In the following, we assume that you are in the source directory of Sage (SAGE_ROOT), obtained either from a source tarball or by cloning a Sage git repository such as https://github.com/sagemath/sage.git, as described in the README <https://github.com/sagemath/sage/#readme>_. In either case, this source directory is actually the main worktree of a local git repository.

Does the source tarball really contain a git folder (to serve as a local git repo)?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from af70d2a to 52b15e6

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

52b15e6src/doc/en/developer/git_setup.rst: Revise guidance on Windows git
mkoeppe commented 2 years ago
comment:35

Replying to @tobiasdiez:

Does the source tarball really contain a git folder (to serve as a local git repo)?

Yes.

tobiasdiez commented 2 years ago
comment:36

I also would propose to make the developer documentation self-contained. I remember that I was super confused when I started, since it was not clear what to do after cloning the repo. Maybe it would be better if the "installation by source" would be linking to the developer docs instead?

mkoeppe commented 2 years ago
comment:37

Larger changes like this should go in a follow up ticket. I won't work on it for Sage 9.5.

mkoeppe commented 2 years ago
comment:38

Replying to @mkoeppe:

Replying to @tobiasdiez:

Does the source tarball really contain a git folder (to serve as a local git repo)?

Yes.

Turns out this one has the remote set to the broken gitlab mirror as well. I'll fix this here in the next commit

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

08e3da4src/bin/sage-env: Update SAGE_REPO_ANONYMOUS to use GitHub
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 52b15e6 to 08e3da4

mkoeppe commented 2 years ago
comment:41

Also, build/bin/sage-clone-source (which is used for creating the git repo in the source tarball) uses origin as the name of the remote. I'll change this to trac to be consistent with the developer documentation.

mkoeppe commented 2 years ago
comment:42

... in #33087.

tobiasdiez commented 2 years ago
comment:44

Replying to @mkoeppe:

Larger changes like this should go in a follow up ticket. I won't work on it for Sage 9.5.

Well, you touched this part of the docs and in my opinion its even less clear now what to do after obtaining the code. As a compromise, maybe add "run configure + make, for more details see installation from source section" for now?

mkoeppe commented 2 years ago
comment:45

It is referring to the installation from source section.

  You will then need to `compile Sage
  <http://doc.sagemath.org/html/en/installation/source.html>`_ in order to use it.

I just removed the duplication of (outdated, short) installation instructions from this place. That's an improvement.

Adding back incomplete short installation instructions ("configure + make") is not an improvement.

tobiasdiez commented 2 years ago
comment:46

Okay, I find a short (but up-to-date) installation instructions helpful for newcomers. It adds quite a barrier if you need to read 5 pages just to get started with compilation.

Other than that, the changes to the docs look good to me. I cannot judge the latest commit, so I leave the ticket on needs review.

Note also that most points of the ticket description are not addressed if I'm not mistaken, so they should probably be moved to new tickets.