Opened 5 years ago

Closed 5 years ago

#18095 closed enhancement (fixed)

Use src/Makefile for building Sage

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.6
Component: build Keywords: sd66
Cc: vdelecroix, fbissey Merged in:
Authors: Jeroen Demeyer Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: 45e6c35 (Commits) Commit: 45e6c3536c0a703f2eb854094d5fac5ab52b8e3e
Dependencies: #17860 Stopgaps:

Description (last modified by jdemeyer)

The auto-generating of src/sage/ext/interpreters/* and src/sage/libs/pari/auto*.pxi should be done by src/Makefile which has proper dependency checking.

To further clean up building of Sage, we remove src/bin/sage-build and move all the logic for building the Sage library to src/Makefile.

Change History (26)

comment:1 Changed 5 years ago by jdemeyer

  • Keywords sd66 added

comment:2 Changed 5 years ago by jdemeyer

  • Summary changed from Use build/deps for auto-generating files to Use src/Makefile for building Sage

comment:3 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/18095

comment:4 Changed 5 years ago by git

  • Commit set to 41755fe8772858c5d94cc851efdb24ec9adfb6f4

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

9f710a2Merge commit '4b071619a00e667d3452a73229de43f231dfe662' into HEAD
41755feRe-organize building of Sage library and auto-generated files

comment:5 Changed 5 years ago by fbissey

  • Cc fbissey added

comment:6 Changed 5 years ago by jdemeyer

  • Dependencies set to #17860
  • Status changed from new to needs_review

comment:7 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:8 follow-up: Changed 5 years ago by mmezzarobba

Am I correct to understand that you removed the ability to do sage-build -b(or sage -b -b, if I read the source correctly, though I don't know if this version ever was intended to work) in order to perform a more aggressive rebuild? I have nothing against removing it, but this ”feature“ was mentioned on sage-support, so perhaps its removal should be a bit more explicit.

Also, perhaps the calls to build_sage "$@" in sage/bin/sage should be changed to just build_sage (and the corresponding shifts removed when they serve no other purpose) for clarity.

(I only had a quick look at the changes and did not even test them.)

comment:9 Changed 5 years ago by mmezzarobba

Another minor point: why did you leave an empty .gitignore in src/sage_setup?

comment:10 in reply to: ↑ 8 ; follow-up: Changed 5 years ago by jdemeyer

Replying to mmezzarobba:

Am I correct to understand that you removed the ability to do sage-build -b(or sage -b -b, if I read the source correctly, though I don't know if this version ever was intended to work) in order to perform a more aggressive rebuild?

Yes, I did that. However, this is no longer needed since ./sage -ba already does that (and this is the documented version and this still works).

this ”feature“ was mentioned on sage-support

Do you have a link?

Also, perhaps the calls to build_sage "$@" in sage/bin/sage should be changed to just build_sage (and the corresponding shifts removed when they serve no other purpose) for clarity.

If you prefer, that can be done.

comment:11 follow-up: Changed 5 years ago by mmezzarobba

Shouldn't the sage make target depend on csage? Or is

all:
        $(MAKE) csage
        $(MAKE) sage

a workaround because the way csage is built makes it hard to set up the dependency without rebuilding csage every time? If that's the case, perhaps add a comment to that effect.

Last edited 5 years ago by mmezzarobba (previous) (diff)

comment:12 in reply to: ↑ 10 ; follow-up: Changed 5 years ago by mmezzarobba

Replying to jdemeyer:

Yes, I did that. However, this is no longer needed since ./sage -ba already does that (and this is the documented version and this still works).

Sure. I was just suggesting to mention the change in the commit messages or ticket description.

this ”feature“ was mentioned on sage-support

Do you have a link?

http://osdir.com/ml/sage-support/2010-01/msg00521.html

Also, perhaps the calls to build_sage "$@" in sage/bin/sage should be changed to just build_sage (and the corresponding shifts removed when they serve no other purpose) for clarity.

If you prefer, that can be done.

I think it is worth doing, but it's up to you.

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

Replying to mmezzarobba:

Shouldn't the sage make target depend on csage?

There is already some logic in build/deps such that when running make, then csage is built before sage.

all:
        $(MAKE) csage
        $(MAKE) sage

a workaround because the way csage is built makes it hard to set up the dependency without rebuilding csage every time?

I wouldn't call it a "workaround", it's more a separation of concerns. The default target all builds both, so there is no dependency issue either.

In any case: I would like to remove csage completely (see #17854), so this issue will disappear anyway.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by mmezzarobba

Replying to jdemeyer:

Replying to mmezzarobba:

Shouldn't the sage make target depend on csage?

There is already some logic in build/deps such that when running make, then csage is built before sage.

all:
        $(MAKE) csage
        $(MAKE) sage

a workaround because the way csage is built makes it hard to set up the dependency without rebuilding csage every time?

I wouldn't call it a "workaround", it's more a separation of concerns. The default target all builds both, so there is no dependency issue either.

I was wondering because you didn't just write all: csage sage.

In any case: I would like to remove csage completely (see #17854), so this issue will disappear anyway.

Ok, fine with me.

comment:15 in reply to: ↑ 12 Changed 5 years ago by jdemeyer

Replying to mmezzarobba:

this ”feature“ was mentioned on sage-support

I see, a post from 2010. In any case, the ability to run src/bin/sage-build will be removed simply because sage-build is removed. But this was never meant to be used directly by users, so I don't think that's an issue.

comment:16 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by jdemeyer

Replying to mmezzarobba:

I was wondering because you didn't just write all: csage sage.

Because that might build csage and sage in parallel.

When make all is done, I do want csage and sage to be built in this order.

comment:17 in reply to: ↑ 16 Changed 5 years ago by mmezzarobba

Replying to jdemeyer:

Replying to mmezzarobba:

I was wondering because you didn't just write all: csage sage.

Because that might build csage and sage in parallel.

When make all is done, I do want csage and sage to be built in this order.

Well, yes, that's precisely what I found strange: that you want them to be built in a particular order but not one to depend on the other.

Anyway, if csage is about to disappear, let's not waste time with that. :-)

comment:18 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:19 Changed 5 years ago by git

  • Commit changed from 41755fe8772858c5d94cc851efdb24ec9adfb6f4 to 3510d4595fc6f35d801d69336e51d15f8bbe4ef4

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

3510d45Minor review fixes

comment:20 Changed 5 years ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

comment:21 follow-up: Changed 5 years ago by mmezzarobba

  • Status changed from positive_review to needs_work

Hmm, I missed something, sorry:

$ make -j4 build    
cd build && \
"../build/pipestatus" \
        "env SAGE_PARALLEL_SPKG_BUILD='' ./install all 2>&1" \
        "tee -a ../logs/install.log"
Nothing to (re)build / all up-to-date.
./sage -b
make[1]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.

It may well be that the warning is benign and the build is still done in parallel, I'm not sure. But in any case the suggested change

diff --git a/Makefile b/Makefile
index 2e8ef6e..f307043 100644
--- a/Makefile
+++ b/Makefile
@@ -20,7 +20,7 @@ build: logs configure
        "../$(PIPE)" \
                "env SAGE_PARALLEL_SPKG_BUILD='$(SAGE_PARALLEL_SPKG_BUILD)' ./install all 2>&1" \
                "tee -a ../logs/install.log"
-       ./sage -b
+       +./sage -b
 
 # Preemptively download all standard upstream source tarballs.
 download:

makes the warning disappear.

comment:22 Changed 5 years ago by git

  • Commit changed from 3510d4595fc6f35d801d69336e51d15f8bbe4ef4 to 45e6c3536c0a703f2eb854094d5fac5ab52b8e3e

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

45e6c35The "./sage -b" make command should be recursive

comment:23 Changed 5 years ago by jdemeyer

  • Status changed from needs_work to positive_review

comment:24 in reply to: ↑ 21 Changed 5 years ago by jdemeyer

Replying to mmezzarobba:

It may well be that the warning is benign and the build is still done in parallel

The build is unfortunately never done in parallel (since ./sage -b uses scons, cython and distutils, which are not aware of make arguments). That's why you're supposed to do MAKE="make -j4" make in Sage, the environment variable can easily be read by Sage.

comment:25 Changed 5 years ago by fbissey

In any case if you have a concern that csage and sage could be built in parallel you should make csage a dependency of sage (made need to add a phony for that to work since csage is not a file).

comment:26 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/18095 to 45e6c3536c0a703f2eb854094d5fac5ab52b8e3e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.