mgansler / mscgen

Automatically exported from code.google.com/p/mscgen
GNU General Public License v2.0
1 stars 0 forks source link

tests (r60) fails if mscgen 0.16 is installed (as /usr/bin/mscgen ) #17

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
The tests assumed that the installed version of mscgen (in /usr/bin/mscgen)
is 0.17 (or compatible) - This leads to two separate failures.

First off, 0.16 does not support the #! approach.

Secondly, test 8 has been modified to catch regressions on the C-style
comments that 0.16 does not support.

Also requiring "backwards compatibility" on a binary output level might
come back to hunt mscgen later. You may want to make the binary comparison
a "maintainer" test (e.g. checking for a env-variable or so before
reporting a failure for a binary mismatch).

I have included a patch that 
 A) tests mscgen from path (in case mscgen was not installed in
/usr/bin/mscgen) 
 B) Works around mscgen 0.16's lack of features (and hopefully also earlier
versions) - This includes explicitly skipping "comparison" on test 8.

and can be applied with 
    patch -d test -p1 < $PATCH_FILE

This introduces grep, cut, cat and sed as requirements (but can be
rewritten to work without sed) - type, echo and test are (as I recall)
shell built-ins.

~Niels

Original issue reported on code.google.com by NThykier@gmail.com on 18 Jul 2009 at 8:01

Attachments:

GoogleCodeExporter commented 8 years ago
That's a good (and fast!) observation.

Thinking about it again, I'm reasonably sure that the command line parsing 
won't fall
apart, so I don't really need that as a regression test.

Regarding changes to the testinput8.msc having regression tests for multi-line
comments, that should only come in when 0.17 is released.  So long as the built
mscgen is ran (i.e. not the installed version), it should be okay.

One thing that does worry me is that the examples hardwire /usr/bin/mscgen.  The
autotools could of course install anywhere, so I'm not sure how to deal with 
that. 
Does it need a post install hook or something to build the examples?

Original comment by Michael....@gmail.com on 18 Jul 2009 at 8:21

GoogleCodeExporter commented 8 years ago
Saturday nights are best spent checking that "the code still compiles and 
runs". ;)

In regards to the autotools, simply rename the examples to "$FILE.in" and 
replace
"/usr/bin" with @@bindir@@ and them to the AC_CONFIG_FILES. The "configure" 
should
generate "$FILE" from "$FILE.in" replacing @@bindir@@ with the correct variable 
and
tha-da... intelligent install location[1]. The "$FILE" will be cleaned by
"distclean"[2] and should probably be listed as example (and perhaps built 
sources).
The "$FILE.in" should be in EXTRA_DIST... at least I believe this will work.

Other than that: Yes, I believe this issue would be solved if the installed 
version
of mscgen is not used in the tests by default (or does not cause the tests to 
fail if
used). I think it is a good idea to optionally use an older version of mscgen 
to warn
about "output" differences (that maintainers can choose to enable), since we 
lack a
way of automatically verify that the output generated is correct.

~Niels

[1] Sample: rename "boxes.msc" to "boxes.msc.in" and change the first line to:
#!@@bindir@@/mscgen -Tpng
 - NB: I am unsure if @@bindir@@ by default expands with a trailing slash.

[2] since configure "created" it, I believe it will remove it as well on a 
distclean
- but I could be wrong, so you may actually have to add a distclean-local rule 
for it.

Original comment by NThykier@gmail.com on 18 Jul 2009 at 9:49

GoogleCodeExporter commented 8 years ago
@@bindir@@ expands to @${exec_prefix}/bin@
@bindir@ expands to ${exec_prefix}/bin, which is better, but still not quite 
what I'm
after...

Is there a syntax to get this recursively expanded?

Original comment by Michael....@gmail.com on 19 Jul 2009 at 10:31

GoogleCodeExporter commented 8 years ago
Hmm... true that, I played around with it and I do not think it is possible to 
have
it expanded further during configure (e.g. whenever try to recursively expand 
it I
get "NONE/bin").

Building it from makefiles is the way - since configure passes the $(bindir) 
variable
to them and they recursively expand it. The following make rule can be used to 
build
the files, just remove the "#!" line from the .in files:

%.msc: %.msc.in
    echo "#!$(bindir)/mscgen -Tpng" | cat - $< > $@

However it seems that automake does not readily support installing 
"examples"... I
will play around a bit with it and see if I cannot "convince" the autotools to 
behave
reasonably (e.g. add a "--examplesdir" switch to configure, set variables 
properly
and pass them to make). 

I will get back to you when I am done wrestling with the autotools.

~Niels

Original comment by NThykier@gmail.com on 19 Jul 2009 at 11:57

GoogleCodeExporter commented 8 years ago
A hacky workaround is to use "#!@prefix@/bin/mscgen -Tpng"...

It's not ideal, but works, at least on the tested version of autotools.

Original comment by Michael....@gmail.com on 19 Jul 2009 at 12:44

GoogleCodeExporter commented 8 years ago
It works as long as a user does not have separate "exec_prefix" from "prefix".
Usually this is true, but it would break if a user made

./configure --prefix=/usr/ --exec-prefix=/usr/local/

or even

./configure --prefix=/usr/ --bindir=/usr/local/bin

The examples would then incorrectly list "/usr/bin/mscgen" when they should be 
using
"/usr/local/bin/mscgen"... again for your average install this is not going to 
be an
issue.

~Niels

Original comment by NThykier@gmail.com on 19 Jul 2009 at 12:59

GoogleCodeExporter commented 8 years ago
I've added a Makefile with rules as you suggest in r64.

So it's just the installation of the examples which needs sorting.

Original comment by Michael....@gmail.com on 19 Jul 2009 at 2:52

GoogleCodeExporter commented 8 years ago
Hey again

I have created some install hooks that are optionally created by configure 
based on
the "WITH_EXAMPLES" environment variable (defaulting to "yes"). I did consider
implementing a full "--with-examples" rule for it... Though if you feel it is
overkill for it, we can just make it always install the examples.

I also got a warning about the %.msc rule being a GNU extension (for make) so I
replaced it with a "compatible" solution; this is the reason for the rename of
"examples/$F.msc.in" to "examples/$F.mscin".

Last but not least I noticed the "-std=gnu99" in src/Makefile.am. This has been
removed and instead I have added a macro in configure.ac to figure out how to 
make
the compiler use the C99 standard. In gcc's case this will be translated to the
-std=gnu99.

I did not add the "AC_PROG_MDKIR_P" macro to configure.ac, since the manual 
says that
AM_INIT calls it[1].

That should cover all the modifications done by this patch.

~Niels
[1] http://www.gnu.org/software/hello/manual/automake/Obsolete-Macros.html

Original comment by NThykier@gmail.com on 19 Jul 2009 at 4:22

Attachments:

GoogleCodeExporter commented 8 years ago
Cool - that patch looks good.

I've made one minor change and accepted it - the install for the examples is 
now done
with $(INSTALL_SCRIPT) so that the examples end up executable.

It's in at r66 if you want to check it.

Original comment by Michael....@gmail.com on 19 Jul 2009 at 5:37

GoogleCodeExporter commented 8 years ago
Hi again

It looks good and runs just fine - installing them executable certainly fits 
them best.

Also, I just noticed two minor things.

A) configure.ac uses deprecated macros, running autoupdate fixes this.
B) autogen.sh can be replaced by "autoreconf" [1]. 

Incidentally, the deprecated macro was caught by using "autoreconf -W all" 

~Niels

[1] Without any arguments it will refresh configure and makefiles. However if 
the
"required" files are missing, it will need "--install[ --symlink]" to install 
them.
See "autoreconf --help" for more info.

Original comment by NThykier@gmail.com on 19 Jul 2009 at 7:30

GoogleCodeExporter commented 8 years ago
This issue was closed by r70.

Original comment by Michael....@gmail.com on 19 Jul 2009 at 7:38