Serialization of setuptools targets in spkg/standard/deps
SnarkBoojum opened this issue · 31 comments
The attached patch implements the serialization of setuptools target through the use of helper stamp targets, as discussed recently on the sage-devel mailing-list
The current situation is that some packages have wrong dependencies to force a serialization.
The patch corrects the dependencies, and forces serialization through stamp targets. Notice that on the mailing-list, the sample code created the stamps in . (ie: spkg/), while the current patch puts them in build/ (ie: spkg/build/), since that seemed more logical.
Apply attachment: trac12994.patch
Depends on #11874
Component: build
Author: R. Andrew Ohana
Reviewer: Jeroen Demeyer
Merged: sage-5.4.beta1
Issue created by migration from https://trac.sagemath.org/ticket/12994
Please clarify or remove the following sentence:
Note: To avoid branching, we haven't given explicit dependencies, but the chain's order is important.
Your patch makes the mistaken assumption that the directory build/ exists. In any case, $(INST) might be a better directory for this.
Seems like your proposal actually makes the deps file more complicated, so I'm not sure I like it...
Why not simply do
$(INST)/$(ZODB): $(INST)/$(TWISTED)
instead of
build/setuptools-serial-1-stamp: $(INST)/$(TWISTED)
touch $@
$(INST)/$(ZODB): build/setuptools-serial-1-stamp
Replying to @jdemeyer:
Please clarify or remove the following sentence:
Note: To avoid branching, we haven't given explicit dependencies, but the chain's order is important.
My patch removes that sentence, so I don't understand what you expect :-/
Replying to @jdemeyer:
Your patch makes the mistaken assumption that the directory build/ exists.
That directory is created in base/prereq-1.0-install (it's even the first thing it does!), so what can happen?
Replying to @jdemeyer:
In any case, $(INST) might be a better directory for this.
I pondered for a while between spkg/build/ and spkg/installed/ indeed, because they're the obvious two possible choices, but finally I opted :
- against spkg/installed/, considering that was where sage documents what features are installed, and you can't consider a stamp file as a feature ;
- for spkg/build/, considering that directory was the one used as scratch space during compilations -- and the stamp files seem to fit that description.
Replying to @jdemeyer:
Seems like your proposal actually makes the deps file more complicated, so I'm not sure I like it...
The current deps works around a problem by cheating on the dependencies : it takes the correct dependencies, and makes them incorrect just so something else doesn't break. My patch is about letting correct things stay correct, and providing the fix for the something which is bad.
A comparison might help : if you bang your head against the wall, it hurts. The current solution is to soften the wall, and my patch is to stop banging.
Could you make your patch using Mercurial? Also, I personally don't like that when done building, the BUILD directory is polluted with 6 empty files.
Replying to @SnarkBoojum:
My patch removes that sentence, so I don't understand what you expect :-/
Actually, your patch simply moves that sentence.
Replying to @SnarkBoojum:
Replying to @jdemeyer:
Replying to @jdemeyer:Your patch makes the mistaken assumption that the directory build/ exists.
That directory is created in base/prereq-1.0-install (it's even the first thing it does!), so what can happen?
Actually, prereq-1.0-install creates SAGE_BUILD_DIR if this variable is set, spkg/build if not. So I guess you should do the same check here: if SAGE_BUILD_DIR is set, use it; otherwise use build/?
The new version of the patch :
- is a mercurial patch ;
- removes the unclear sentence I hadn't written but merely moved ;
- respects SAGE_BUILD_DIR ;
- cleans the stamp files after the build is complete.
Sigh. I had hand-edited the patch before uploading because I wondered why I had written rm -f
Attachment: setuptools-serial.patch.gz
Patch fixing the issue
What is the current status of this ticket? Is it ready for review?
Attachment: trac12994.patch.gz
apply only this patch to the root repo
IMHO making these stamps are unnecessary. I've posted a patch containing my solution, which simply separates out the linearization, and puts it at the bottom of deps.
Author: R. Andrew Ohana
Your patch doesn't solve the problem of lying dependencies. Those packages don't depend on each other, so the deps file must not say otherwise.
Replying to @SnarkBoojum:
Your patch doesn't solve the problem of lying dependencies. Those packages don't depend on each other, so the deps file must not say otherwise.
Your patch also lies about the dependencies, it just does so implicitly rather than explicitly. As far as I know, you can't force the linearization without either implicitly or explicitly specifying a dependency. I think the cleaner solution is to use explicit dependencies to force the linearization, but to keep these separate from the real dependencies.
My patch doesn't lie : it explicitly adds virtual dependencies, and makes it very clear they are virtual. Your patch doesn't bring anything to the table, as it basically does the same as what is currently done : explicit bogus dependencies between packages.
I have used a script to visualize dependencies between sage packages ; the resulting graph had strange edges. I grepped, and indeed I saw the very explicit rules giving those edges, so my script was right. I checked the code, and it was pretty clear those rules had nothing to do there. I checked deps again by directly reading it, and found the comments stating they were bogus. I claim this isn't a normal situation, and should be fixed, hence the ticket -- and hence my patch.
With it, you can use a script, grep or directly read -- it doesn't matter, you can't miss that the added edges (and vertexes) only deal with the problem of serializing setuptools packages.
I prefer the second patch, because it's simpler while it does essentially the same thing.
Just wondering: why did you reorder the dependencies in that way?
The Sage library depends on jinja2 to build, so maybe it's best to build jinja2 first.
Replying to @jdemeyer:
Just wondering: why did you reorder the dependencies in that way?
Just based off of the dependencies of the packages -- I placed the ones with fewer dependencies earlier in the serial build. I'm not really attached to the ordering I put, I just figured there might be a little less waiting.
I'm looking into fixing this issue in setuptools/distribute directly (I already got my hands dirty in the code fixing another issue), so hopefully this becomes irrelevant.
Replying to @ohanar:
Replying to @jdemeyer:
Just wondering: why did you reorder the dependencies in that way?
Just based off of the dependencies of the packages -- I placed the ones with fewer dependencies earlier in the serial build. I'm not really attached to the ordering I put, I just figured there might be a little less waiting.
The path PYTHON -> DOCUTILS -> JINJA2 is faster than
PYHTON -> ZODB -> SQLALCHEMY -> PYGMENTS -> JINJA2.
The build time for ZODB and SQLALCHEMY doesn't matter because nothing depends on it.
Of course it's a detail, but since you were reordering anyway...
Replying to @jdemeyer:
The path PYTHON -> DOCUTILS -> JINJA2 is faster than
PYHTON -> ZODB -> SQLALCHEMY -> PYGMENTS -> JINJA2.The build time for ZODB and SQLALCHEMY doesn't matter because nothing depends on it.
Sure, but http://wstein.org/home/ohanar/spkgs/setuptools-0.6.16.p1.spkg now includes a patch that fixes the issue with setuptools, and allows for parallel use of setuptools. We still, for the time being, have to have sagenb be the last setuptools spkg to build until #13197 is rebased off of this spkg.
use with spkg
Attachment: trac12994-spkg-fix.patch.gz
Replying to @jdemeyer:
I prefer the second patch, because it's simpler while it does essentially the same thing.
The second patch basically does nothing at all : it serializes things precisely like I complain shouldn't be done, by adding wrong but correct-looking edges in the dependency graph.
This is a typical application of the quote : “For every problem there is a solution which is simple, clean and wrong.”
If that problem goes away by getting the original issue down, that's perfect : my goal isn't to make my patch go in -- it's to get things right : there must not be a "$(INST)/$(FOO):
Replying to @ohanar:
Sure, but http://wstein.org/home/ohanar/spkgs/setuptools-0.6.16.p1.spkg now includes a patch that fixes the issue with setuptools, and allows for parallel use of setuptools. We still, for the time being, have to have sagenb be the last setuptools spkg to build until #13197 is rebased off of this spkg.
Does this spkg belong to this ticket or a different ticket? If the former, add it in the ticket description. If the latter, add the correct Dependency of the ticket.
Replying to @SnarkBoojum:
The second patch basically does nothing at all : it serializes things precisely like I complain shouldn't be done, by adding wrong but correct-looking edges in the dependency graph.
This is a typical application of the quote : “For every problem there is a solution which is simple, clean and wrong.”
If that problem goes away by getting the original issue down, that's perfect : my goal isn't to make my patch go in -- it's to get things right : there must not be a "$(INST)/$(FOO):
$(INST)/$ (BAR)" rule in deps between actual packages which doesn't correspond to an actual dependency.
I think we all made our point and have to realize that we don't agree.
Replying to @jdemeyer:
Does this spkg belong to this ticket or a different ticket? If the former, add it in the ticket description. If the latter, add the correct Dependency of the ticket.
It should belong to a different ticket, I just hadn't made one last night, and I needed to go to bed. This spkg (and corresponding patch) are now at #13201.
I would say: let's not depend on #13201 here. Instead, you could fix spkg/standard/deps at #13201. So I assume that attachment: trac12994.patch is still the subject of review.
Reviewer: Jeroen Demeyer
Description changed:
---
+++
@@ -4,4 +4,4 @@
The patch corrects the dependencies, and forces serialization through stamp targets. Notice that on the mailing-list, the sample code created the stamps in . (ie: spkg/), while the current patch puts them in build/ (ie: spkg/build/), since that seemed more logical.
-I checked on sage.math that parallel compilation works, and that compilation with checks passes.
+**Apply** [attachment: trac12994.patch](https://github.com/sagemath/sage-prod/files/10655589/trac12994.patch.gz)Merged: sage-5.4.beta1