On Tue, Jan 30, 2018 at 8:45 PM, Jeroen Demeyer <j.deme...@ugent.be> 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.
