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:  sage6.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 )
The autogenerating 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/sagebuild
and move all the logic for building the Sage library to src/Makefile
.
Change History (26)
comment:1 Changed 5 years ago by
 Keywords sd66 added
comment:2 Changed 5 years ago by
 Summary changed from Use build/deps for autogenerating files to Use src/Makefile for building Sage
comment:3 Changed 5 years ago by
 Branch set to u/jdemeyer/ticket/18095
comment:4 Changed 5 years ago by
 Commit set to 41755fe8772858c5d94cc851efdb24ec9adfb6f4
comment:5 Changed 5 years ago by
 Cc fbissey added
comment:6 Changed 5 years ago by
 Dependencies set to #17860
 Status changed from new to needs_review
comment:7 Changed 5 years ago by
 Description modified (diff)
comment:8 followup: ↓ 10 Changed 5 years ago by
Am I correct to understand that you removed the ability to do sagebuild 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 sagesupport, 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 shift
s 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
Another minor point: why did you leave an empty .gitignore
in src/sage_setup
?
comment:10 in reply to: ↑ 8 ; followup: ↓ 12 Changed 5 years ago by
Replying to mmezzarobba:
Am I correct to understand that you removed the ability to do
sagebuild b
(orsage 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 sagesupport
Do you have a link?
Also, perhaps the calls to
build_sage "$@"
insage/bin/sage
should be changed to justbuild_sage
(and the correspondingshift
s removed when they serve no other purpose) for clarity.
If you prefer, that can be done.
comment:11 followup: ↓ 13 Changed 5 years ago by
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.
comment:12 in reply to: ↑ 10 ; followup: ↓ 15 Changed 5 years ago by
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 sagesupport
Do you have a link?
http://osdir.com/ml/sagesupport/201001/msg00521.html
Also, perhaps the calls to
build_sage "$@"
insage/bin/sage
should be changed to justbuild_sage
(and the correspondingshift
s 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 ; followup: ↓ 14 Changed 5 years ago by
Replying to mmezzarobba:
Shouldn't the
sage
make target depend oncsage
?
There is already some logic in build/deps
such that when running make
, then csage
is built before sage
.
all: $(MAKE) csage $(MAKE) sagea workaround because the way
csage
is built makes it hard to set up the dependency without rebuildingcsage
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 ; followup: ↓ 16 Changed 5 years ago by
Replying to jdemeyer:
Replying to mmezzarobba:
Shouldn't the
sage
make target depend oncsage
?There is already some logic in
build/deps
such that when runningmake
, thencsage
is built beforesage
.all: $(MAKE) csage $(MAKE) sagea workaround because the way
csage
is built makes it hard to set up the dependency without rebuildingcsage
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
Replying to mmezzarobba:
this ”feature“ was mentioned on sagesupport
I see, a post from 2010. In any case, the ability to run src/bin/sagebuild
will be removed simply because sagebuild
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 ; followup: ↓ 17 Changed 5 years ago by
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
Replying to jdemeyer:
Replying to mmezzarobba:
I was wondering because you didn't just write
all: csage sage
.Because that might build
csage
andsage
in parallel.When
make all
is done, I do wantcsage
andsage
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
 Description modified (diff)
comment:19 Changed 5 years ago by
 Commit changed from 41755fe8772858c5d94cc851efdb24ec9adfb6f4 to 3510d4595fc6f35d801d69336e51d15f8bbe4ef4
Branch pushed to git repo; I updated commit sha1. New commits:
3510d45  Minor review fixes

comment:20 Changed 5 years ago by
 Reviewers set to Marc Mezzarobba
 Status changed from needs_review to positive_review
comment:21 followup: ↓ 24 Changed 5 years ago by
 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 uptodate. ./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
 Commit changed from 3510d4595fc6f35d801d69336e51d15f8bbe4ef4 to 45e6c3536c0a703f2eb854094d5fac5ab52b8e3e
Branch pushed to git repo; I updated commit sha1. New commits:
45e6c35  The "./sage b" make command should be recursive

comment:23 Changed 5 years ago by
 Status changed from needs_work to positive_review
comment:24 in reply to: ↑ 21 Changed 5 years ago by
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
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
 Branch changed from u/jdemeyer/ticket/18095 to 45e6c3536c0a703f2eb854094d5fac5ab52b8e3e
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Merge commit '4b071619a00e667d3452a73229de43f231dfe662' into HEAD
Reorganize building of Sage library and autogenerated files