[Nut-upsdev] [Nut-upsuser] Belkin F6C1100-UNV

[Peter Selinger]

Hi Eric,

your patch (committed with lightening speed) differs in many respects
from what I was discussing below.  This goes all the way down to the
fact that you included non-portabile shell code, lots of local changes
to individual source files that we had previously said we wanted to
avoid. Not to mention: the patch does not actually work, due to an
incorrect use of $(topdir), which should be $(top_srcdir).
And if you do "make dist" and then build from the tarball, you get
"2.1.0 (exported)" as the version number, which is not what we wanted.

Developers: I think we should audit patches more carefully before
committing, especially with a testing release coming up, and
especially if the discussion on the mailing list on how to best
resolve some issue is still ongoing.  Please resist pressure to do
it in a hurry. 

Eric: could you reply to my comments below and discuss why you didn't
take them into consideration? 

-- Peter



Peter Selinger wrote:
> 
> Eric S. Raymond wrote:
> > 
> > Peter Selinger <selinger at mathstat.dal.ca>:
> > > Indeed. By the way, at the risk of stating the obvious; it's not just
> > > drivers that should display the SVN revision, but all other NUT
> > > components that display version numbers as well - server, clients,
> > > upsmon etc.
> > 
> > Noted.  I think the way to accomplish that will be to actually modify the 
> > value of UPS_VERSION to include the revision.
> 
> I agree. I've been thinking more about this. I think we agreed that
> this should be done centrally if at all possible, i.e., by modifying
> only Makefiles and such.
> 
> I thought about the pros and cons of using the config.h mechanism
> vs. the CFLAGS=-D... mechanism for this purpose. We currently use
> config.h for all compile-time configurations of C code. This is good
> for a number of reasons. However, I don't think it would work for the
> UPS_VERSION variable, because (I think) config.h only gets updated
> at configure time, not at make time. configure does not rerun
> automatically after each svn update. 
> 
> So the UPS_VERSION variable has to be removed from config.h and added
> into CFLAGS. However, this leads to another problem, namely, how to
> cause the C sources to be recompiled when the source changes. The
> mechanism you suggest certainly works, *if* each source file that uses
> UPS_VERSION has an explicit dependency on $top_srcdir/revision-stamp.
> Adding such a dependency requires going through each Makefile and all
> sources, which I would like to avoid. 
> 
> Another possibility would be to create a $top_srcdir/revision-stamp.h
> that could be #included by include/config.h; in this case, the
> automatic dependency tracking would be able to find this
> dependency. But config.h.in is auto-generated by autoheader, and I
> don't think there is a way to make it include another file. 
> A possible workaround would be to rename config.h as autoconfig.h, and
> make a new file config.h which just #includes the two files
> autoconfig.h and $top_srcdir/revision-stamp.h. But this seems
> unelegant. 
> 
> I also thought about another issue we discussed, namely how to
> enable/disable the revision number display, so that it's displayed
> when someone builds from SVN, but not displayed when someone builds
> from a release tarball. Adding an --enable-display-revision switch is
> perhaps not desirable, since this should be a maintainer option, and
> not an installer option. 
> 
> Perhaps the easiest way is to let the Makefile test the output of
> svnversion. If it's "exported", then don't display the revision
> number, otherwise, do. The only case that this does not treat
> correctly is if someone builds their own tarball from an unstable SVN
> version, and then builds from the tarball instead of directly from
> SVN. People don't normally do this; however, our autobuilder might do
> this if we decide to build nightly tarballs. In that case, an extra
> trick will be needed to get the autobuilder to build revisioned
> tarballs. 
> 
> A final point on portability. To be portable, all shell scripts
> (including in configure.in and Makefiles) should use the "test" syntax
> and not the [ ... ] syntax for boolean tests. The latter may not work
> with archaic versions of sh (you would be surprised at the kinds of
> systems on which people run NUT). Also, the syntax
> 
> test x$LEFT = x$RIGHT, 
> 
> while an old Unix tradition, is ugly: it works correctly for empty
> strings, but not for strings that contain spaces. I have been
> replacing this by
> 
> test "$LEFT" = "$RIGHT"
> 
> in NUT code whereever I have been seeing it. The latter syntax works
> correctly in all cases, and even works with `...` quotation, i.e., you
> can write "`some command`".
> 
> -- Peter
> 
> 
> 
> _______________________________________________
> Nut-upsdev mailing list
> Nut-upsdev at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev
>