Nice bugs, very tasty and crunchy, in the build system come from /bin/sh not being bash on some systems. And different versions of Make are sometimes a lot of fun, too.
Having said this, I much prefer short to verbose, and modular to "one huge file with everything there" design. On Wednesday, January 31, 2018 at 11:21:34 AM UTC, Erik Bray wrote: > > On Tue, Jan 30, 2018 at 8:45 PM, Jeroen Demeyer <j.de...@ugent.be > <javascript:>> wrote: > > On 2018-01-30 13:19, Erik Bray wrote: > >> > >> I think the resulting Makefile, and its > >> template, are easier to understand for one. > > > > > > This is unfortunately the part where I disagree and it is also the > reason > > why I am against the current patch. The resulting Makefile uses some > macro > > constructions which make it hard to understand: the Makefile does not > > contain the actual rules for building the packages, only macros. Most of > the > > time, this might be fine. But when some obscure bug in the build system > > comes around, not seeing the actual rules nor being able to edit them is > > going to make things difficult. > > > > I can imagine that there are things to be improved in the way how the > > Makefile is generated. So I'm not against improvements or changes in > > general. But I am against changes which make the resulting Makefile > harder > > to understand. > > I'm not going to say I didn't expect pushback from you on this, but I > feel like this is not a real argument. I'll get back to this in more > detail later but I can hardly see how using a macro in a loop makes > something harder to understand. There's almost no other context in > which you would normally "unroll" some repetitive text like this if > you didn't have to, and there's no reason this needs to be an > exception. In fact, the old code was still essentially doing the same > thing, but in the form of some ugly shell code buried in a > configure.ac and not even in the makefile itself. An apt analogy > would be a web server generating HTML by printf statements versus > using a template format intended for HTML. In the case of make the > template syntax is, admittedly, a little ugly, but we do the best we > can with the tools we have, and use them as intended. > > Now, I thought that the comments I included in the file made it > reasonably clear what was going on, but obviously it wasn't so maybe > if I focused on some improvements there it would help. Looking at it > again, it's apparent that even my explanations are a little > template-y, so maybe it would also help to have some more concrete > examples as well. > > Your only point about debugging is extremely hypothetical--exactly > what kind of "obscure bug in the build system" do you anticipate being > obscured by this? As you can imagine, in writing this I didn't get it > exactly right on the first go and had to work on it a bit, but this > didn't make any more difficult to debug. If you expand even just one > of those macros you can see the "actual rules" perfectly fine. You > can either do this by hand, or you can use a little debug helper that > I had in originally and took out of the final version (but perhaps I > could restore) that looks like: > > ifneq "$(DEBUG_RULES)" "" > $(foreach pkgname,$(NORMAL_PACKAGES),\ > $(info $(call NORMAL_PACKAGE_templ,$(pkgname)))) > endif > > This is almost exactly the same as the call that generates the actual > rules, but $(eval ...) is replaced by $(info ...) which simply prints > the expanded template. Then run > > $ make -f build/make/Makefile -n DEBUG_RULES=1 > > and you get output like: > > $(INST)/4ti2-$(vers_4ti2): $(foreach dep,$(deps_4ti2),$(inst_$(dep))) > +sage-logger -p 'sage-spkg 4ti2-$(vers_4ti2)' > '/4ti2-$(vers_4ti2).log' > > 4ti2: $(INST)/4ti2-$(vers_4ti2) > > 4ti2-clean: > rm -rf $(INST)/4ti2-$(vers_4ti2) > > > ... > > These aren't so different from the rules you see in the current Makefile: > > $(INST)/4ti2-1.6.7: $(inst_zlib) $(MP_LIBRARY) $(inst_glpk) > +$(AM_V_at)sage-logger -p '$(SAGE_SPKG) 4ti2-1.6.7' > '$(SAGE_LOGS)/4ti2-1.6.7.log' > > 4ti2: $(INST)/4ti2-1.6.7 > > 4ti2-clean: > rm -f $(INST)/4ti2-1.6.7 > > The only differences are that there's a variable for the package > version $(vers_<packagename>), and there's also a variable for the > package's dependencies $(deps_<packagename>). The latter gets > expanded into the variables for each of it's dependency's stamp files, > so whereas this was previously "$(inst_zlib) $(MP_LIBRARY) > $(inst_glpk)" we now have: > > deps_4ti2 = zlib $(MP_LIBRARY) glpk > > so in the rule this gets expanded to: > > $(inst_zlib) $(inst_$(MP_LIBRARY)) $(inst_glpk) => $(inst_zlib) > $(inst_mpir) $(inst_glpk) (for example where MP_LIBRARY=mpir). > > It's unfortunate that the DEBUG_RULES doesn't demonstrate this > additional expansion. It could with a little more elbow grease but I > don't think it's necessary because it's very predictable. > > In summary, I think it's strange to complain about this with autoconf > itself is little more than a massive pile of text macros and is *much* > harder to understand and to debug than this. > > Best, > E > -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
