Mike Kupfer wrote: > Thanks for the review! > >>>>>> "Mark" == Mark J Nelson <Mark.J.Nelson at Sun.COM> writes: > > Mark> Should the devref repo have an explicit .hgignore file for > Mark> rawchunks and cleanchunks dirs? > > Well, it means that changes to the temporary directory names will > require makefile and .hgignore updates. Given the size of the > workspace, I'm inclined to think it's not worth doing, though I'm not > adamantly opposed.
I'm OK with "not enough output to worry about." > Mark> 99-100: this can go away now > > Fixed. Thanks. > Mark> 105: without the $(ALLFILES) dependency, would this be better as a > Mark> .xml.html suffix rule? > > I tried that and got this error: > > make: Fatal error: Don't know how to make target `devref.html' > > I don't know why, and I can't say it's high on my list of things to look > into at the moment... :-/ Huh. Ok. Probably also not worth extra time. > Mark> 108: It seems a little bit confusing for "all" to not depend on > Mark> "chunks." Isn't building this for posting a common scenario? > > Good point. In fact, that's probably the primary scenario, so I've > changed the default target to be (just) "chunks". People that want a > single HTML file will need to do "make devref.html". Thanks, that sounds sensible. > Mark> 116/123: the $(CLEANCHUNKDIR) dependency seems like it might be a > Mark> little bit misplaced, in that it's part of the dependency chain of > Mark> rawchunks instead of $(ONCHUNKS). Seems like there could be a > Mark> race condition for a fast, parallel build, where something could > Mark> fire the actions on 142 (and therefore 136) before 123? > > This is serial make (no .PARALLEL target). IMO it's not big enough to > be worth trying to parallelize. I still think the dependencies should sort correctly, though. I don't think there's a guarantee for which target will be built first, is there? > Mark> ...but now that I look at it a little bit more, the rawchunks > Mark> stuff is strictly an intermediate product. Instead of having raw > Mark> and clean chunk dirs, why not use xargs to invoke fixup.py on each > Mark> chunk in turn at the end of the rule on line 117-118? > > I don't understand what you're suggesting. xsltproc runs once and > produces a bunch of HTML files whose contents and names are close to, > but not exactly, what we want. fixup.py cleans it up *except* for > combining ch03b1.html and ch03b2.html into ch03b.html, and renaming > apa.html to glossary.html. > > Would the flow be easier to understand if I used fewer intermediate > targets and more rules per target? That might also let me get rid of > one intermediate directory. That's actually exactly what I was suggesting. Instead of running xsltproc to create the various raw/*.html files, and then having the rule for clean/blah.html depend on raw/blah.html, just add the fixup.py invocation after xsltproc. > Mark> (Generally, a $(TGTDIR)/% target with a dynamically created > Mark> $(TGTDIR) should depend on both $(TGTDIR) and $(SRCDIR)/%.) > > I've come to the conclusion that this causes more trouble than it's > worth. See item 3 at http://bugs.opensolaris.org/view_bug.do?bug_id=6541200. > > Instead, there should be a separate "phase" in the makefile that ensures > the existence of target directories. That's what I've done here with > the "chunktmpdirs" target. This is probably obviated if you eliminate the intermediate directory. > Mark> 125-126 and 128-129: these can be combined into a single action if > Mark> you use $@ in the rule. > > Done. Thanks. > Mark> The "-p" option to mkdir doesn't hurt anything, but isn't needed. > > I left in the -p, in case someone decides in the future that the > temporary directories should nest further down in the directory tree. Ok. > Mark> 150: I generally don't like wildcards in clean/clobber targets, at > Mark> least not where people might be doing active work. And isn't the > Mark> "*~" an artifact of your editor of choice, rather than the build > Mark> process? > > I removed the "*~". Yes, it's an artifact of my editor, though it's > also a common idiom for "make clean". > > For some reason fixup.pyc is no longer being created, so I removed *.pyc > from the "clean" target, too. Permissions? It should be there, somewhere, I think. > I changed the *.html to > > rm -f devref.html > rm -f index.html ch*.html ch*s*.html apa.html > > The exact chapter and section file names depend on the content in the > XML file. Trying to keep the makefile consistent with the XML here > seems more of a hassle than it's worth. Ok. > Let me know what you think about the intermediate targets and > directories (rawchunks et al, as discussed above). I'm in favor of eliminating the intermediate (rawchunks) directory. --Mark
