Opened 9 years ago

Closed 9 years ago

#12994 closed enhancement (fixed)

Serialization of setuptools targets in spkg/standard/deps

Reported by: Snark Owned by: GeorgSWeber
Priority: minor Milestone: sage-5.4
Component: build Keywords:
Cc: Merged in: sage-5.4.beta1
Authors: R. Andrew Ohana Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11874 Stopgaps:

Status badges

Description (last modified by jdemeyer)

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 trac12994.patch

Attachments (3)

setuptools-serial.patch (4.9 KB) - added by Snark 9 years ago.
Patch fixing the issue
trac12994.patch (3.5 KB) - added by ohanar 9 years ago.
apply only this patch to the root repo
trac12994-spkg-fix.patch (3.5 KB) - added by ohanar 9 years ago.
use with spkg

Download all attachments as: .zip

Change History (30)

comment:1 Changed 9 years ago by 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.

comment:2 Changed 9 years ago by jdemeyer

Your patch makes the mistaken assumption that the directory build/ exists. In any case, $(INST) might be a better directory for this.

comment:3 Changed 9 years ago by jdemeyer

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

comment:4 follow-ups: Changed 9 years ago by Snark

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 jdemyer:

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.

comment:5 Changed 9 years ago by jhpalmieri

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.

comment:6 in reply to: ↑ 4 Changed 9 years ago by jdemeyer

Replying to Snark:

My patch removes that sentence, so I don't understand what you expect :-/

Actually, your patch simply moves that sentence.

comment:7 in reply to: ↑ 4 Changed 9 years ago by jhpalmieri

Replying to Snark:

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/?

comment:8 Changed 9 years ago by Snark

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.

comment:9 Changed 9 years ago by Snark

Sigh. I had hand-edited the patch before uploading because I wondered why I had written rm -f $SAGE_BUILD_DIR/*stamp and not rm -f $(SAGE_BUILD_DIR)/*stamp -- in fact I shouldn't have done that, as it breaks the stamp files cleaning. Let me put up a patch that has been tested and not stupidly modified by hand afterwards.

Changed 9 years ago by Snark

Patch fixing the issue

comment:10 Changed 9 years ago by ohanar

What is the current status of this ticket? Is it ready for review?

Changed 9 years ago by ohanar

apply only this patch to the root repo

comment:11 Changed 9 years ago by ohanar

  • Authors set to R. Andrew Ohana
  • Dependencies set to #11874
  • Priority changed from trivial to minor
  • Status changed from new to needs_review

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.

comment:12 follow-up: Changed 9 years ago by Snark

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.

comment:13 in reply to: ↑ 12 Changed 9 years ago by ohanar

Replying to Snark:

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.

comment:14 Changed 9 years ago by Snark

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.

comment:15 follow-ups: Changed 9 years ago by jdemeyer

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.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 9 years ago by 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.

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.

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

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...

comment:18 in reply to: ↑ 17 ; follow-up: Changed 9 years ago by ohanar

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.

Changed 9 years ago by ohanar

use with spkg

comment:19 in reply to: ↑ 15 ; follow-up: Changed 9 years ago by Snark

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): $(INST)/$(BAR)" rule in deps between actual packages which doesn't correspond to an actual dependency.

comment:20 in reply to: ↑ 18 ; follow-up: Changed 9 years ago by jdemeyer

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.

comment:21 in reply to: ↑ 19 Changed 9 years ago by jdemeyer

Replying to Snark:

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.

comment:22 in reply to: ↑ 20 Changed 9 years ago by ohanar

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.

Last edited 9 years ago by ohanar (previous) (diff)

comment:23 Changed 9 years ago by jdemeyer

I would say: let's not depend on #13201 here. Instead, you could fix spkg/standard/deps at #13201. So I assume that trac12994.patch is still the subject of review.

comment:24 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers set to Jeroen Demeyer

comment:25 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:26 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.3 to sage-5.4

comment:27 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.4.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.