Re: Partial linking of .lo objects, rev 2 (CVS HEAD)

[Ralf Wildenhues]

Hi Marc,

Sorry for the delay.  I have actually started to improve upon your patch
myself, but then got sidetracked; so maybe it's better to present my
current analysis to you, to allow you to pick it up from here.

* Marc Alff wrote on Sun, Apr 09, 2006 at 06:10:35PM CEST:
> 
> Trying again with reloadables ...
> 
> This patch (still proposed) is ...
> - based on CVS HEAD,

Thanks.

> - takes into account all the comments to date,
> - defines functional tests (compile, link, inspect the resulting binary)
> - uses Autotest

Good.  It's probably ok to merge all the tests into one file, and
simplify them somewhat.

The whole patch looks much better already.  Comments:
- the patch can be simplified to use func_write_libtool_object now.
- $libobj needs to be removed before linking, else an old leftover
  may confuse `make' in an error case.
- for the same reason, $libobj needs to be created atomically
  (done with `mv') (this is done by using func_write_libtool_object).
- LDFLAGS may not be overridden in the test suite, only appended to
  (some vital flag may have to remain in there, like -m64)
- We may need to rethink whether LDFLAGS or the contents of a different
  variable should be passed to the reload line;
  There are traps to avoid here in several directions; I haven't thought
  through this completely; probably necessary to just filter out a few
  flags we can allow through, like -64 or so.

- .libs needs to be replaced with $objdir in the tests
- I believe you cannot assume `strings' to be present (unsure about
  this), so it would be prudent to test this.
- `! test' is not portable, but `test !' is.
- Please change `cat file | grep ..'  ->  `grep .. file'
- Please use the GNU CodingStyle indenting (a space before '(', and no
  spaces before ';', ')', etc.  I don't like the GCS much either, but
  I dislike a wild mixture of styles even more.

Corner cases:
- some objects have been compiled PIC-only, then the corresponding
  objects will be missing in the output.  I think this is a bug in the
  current patch.

In the test suite parts you posted, there is a lot of duplication; that
is difficult to maintain cleanly.  Luckily, there are ways out of this:
use m4 macros to define common parts, and merge similar tests into one,
by using shell loops and such.  (Look at some other tests for examples,
convenience.at or static.at; some useful macros are in testsuite.at.)


I don't like the testing with "strings" for another reason: that's not
what you would do with the output in real projects; instead, you would
link the output into some program, or library.  So that is what the
tests should do, verifying that the needed symbols are present.  What's
more, if you're testing for non-presence, the symbol names should be
sufficiently unusual that they won't appear for unrelated reasons.
(Some systems create quite a number of automatic symbols in object
files).

Some more random comments:

| # This test requires :
| # - "enable static libraries"
| 
| AT_CHECK(
|    [($LIBTOOL --features | grep "enable static libraries" ) || exit 77],
|    [0],
|    [ignore],
|    [ignore])

You could just have one test that does --tag=disable-shared; since the
user may want to do this, it's exactly what the test should try.
FWIW, I'd prefer a bit more vertically-compact code, like this:

AT_CHECK([$LIBTOOL --features | grep "enable static libraries" || exit 77],
         [0], [ignore], [ignore])

> The code change in ltmain.m4sh is localized to the <reload_cmds> area,
> but is more extensive, so the diff will probably be real hard to read.

No, that's fine.

> All the parts that needs special review are commented with the keyword 
> "REVIEW-ME", to facilitate inspection.

That's not really necessary; it doesn't matter much, but by virtue of
using "diff" it's already clear which parts have changed.  (IOW, most
of the time I'd read the patch, only when context is necessary I'd read
the resulting code.)

> +#ifdef PIC
> +   "PIC defined"
> +#else
> +   "PIC not defined"
> +#endif

Not all systems have -DPIC; you can find this out by inspecting
 libtool --features | grep "^pic_flag="

Oh, tests of C++ functionality (including templates, and constructors),
would be great, but are of course completely optional.  If you look at
this, be sure to look at those bits of template support that CVS Libtool
has, including tests/template.at.

Cheers,
Ralf