libtool-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH, take 3][cygwin|mingw] Control where win32 DLLs get installed


From: Dave Korn
Subject: Re: [PATCH, take 3][cygwin|mingw] Control where win32 DLLs get installed.
Date: Thu, 13 Aug 2009 15:10:11 +0100
User-agent: Thunderbird 2.0.0.17 (Windows/20080914)

Ralf Wildenhues wrote:
> Hello Dave,
> 
> * Dave Korn wrote on Tue, Aug 11, 2009 at 09:07:12AM CEST:
>>   Well, the bindir option exists only to support PE DLLs,
> 
> Bzzt.  First error.  If libtool provides -bindir, then it should accept
> it on every system, [ ... ]  Of course on non-PE
> systems, it should just ignore the option silently.

  That's exactly what it does already, as I thought I mentioned upthread.

> So it's only the PE-specific bits of the test that should be skipped on
> systems where they don't apply.
> 
> Thus, bindir.at is a sensible name.  

  I can rename it and adjust the tests so they run on all platforms, but make
sure the library /doesn't/ get installed to bindir on non-PE platforms.  Ok?

> Do you intend for Automake to pass
> -bindir to libtool --mode=link invocations automatically (maybe for
> <foo>_LTLIBRARIES with <foo> not equal to lib or libexec)?

  I intend the maintainer to trivially add "-bindir $(bindir)" anywhere in
their makefiles that they would be adding "-no-undefined" to cause DLLs to be
built.  Sure I mentioned that one already, didn't I?

> * Dave Korn wrote on Tue, Aug 11, 2009 at 10:35:28AM CEST:
>>   All rebuilt OK.  Checked docs with "make dvi info pdf" and viewing the
>> generated file.  Testsuite re-run is still in progress, but I already checked
>> that the new tests all still pass, so unless I post back saying otherwise in 
>> the
>> next couple of hours, assume the lot completed without regressions.
> 
> Tested on what system(s)?

i686-pc-cygwin.  Could do a linux run as well if you like.

> BTW, even if the part going into GCC is covered by your GCC assignment,
> the rest is still sufficiently nontrivial to warrant an assignment.

  Gah, of course, forgot about the testcase.  Oh well, I didn't delay sending
off for the paperwork based on that theory, so we've not lost any time.

>> diff --git a/Makefile.am b/Makefile.am
>> old mode 100644
>> new mode 100755
> 
> Your files suffer from mode changes.  They are of course not acceptable,
> though I understand they are a w32 artifact.  Can git be made to ignore
> them for you?

  I have no idea what's going on here.  The modes of the files haven't changed
a bit according to 'ls', so cygwin git must be acting up.  If I was to end up
applying the patch myself I'd probably have to check out libtool on a linux
box and commit it there.

> tests/bindir.at, as already noted above; and as this is about
> 'libtool' (the script) API, please sort it alongside the other API
> tests, preferably before or after cwrapper.at.  Thanks.

  Okeydokey; couldn't tell if there was a grouping and ordering in that list
or not.

>> --- a/doc/libtool.texi
>> +++ b/doc/libtool.texi
>> @@ -1376,6 +1376,15 @@ Tries to avoid versioning (@pxref{Versioning}) for 
>> libraries and modules,
>>  i.e.@: no version information is stored and no symbolic links are created.
>>  If the platform requires versioning, this option has no effect.
>>  
>> address@hidden -bindir
>> +When linking a DLL for Windows or another PE platform, this option tells
> 
> What does this have to do with PE?

  PE is the object file format.  There is no other object file format that has
DLLs, and no DSOs other than DLLs require installation to $PATH.  It would
seem strange to suggest that this would work with ELF DLLs on Linux, for
example, since there are no such things!

>  All this is about is that there is
> no real, independent $shlibpath_var beside PATH.  I'm OK with mentioning
> that Windows is the sole current user of this, but please let's word
> this in a way that doesn't require us to change the interface if some
> other system requires it, too.  Ideally, neither the text.

  I really and honestly cannot imagine any other possible use case for this
functionality.  Still, if you want to suggest some more generic wording, just
tell me what you'd like it to say and I'll paste it in.

>> +# func_relative_path libdir bindir
>> +# generates a relative path from LIBDIR to BINDIR, intended
>> +# for supporting installation of windows DLLs into -bindir.
> 
> gnulib-tool has a function func_relativize.  Can we just reuse that
> (and make it fast)?  Failing that, did you write this function from
> scratch or took it from anywhere else.

  I wrote it from scratch, then Chuck showed me how to do the shell script
portability stuff on this list.  The tests cover all corner-cases and it works
and has no conceivable copyright or licensing snags.  I think those are
reasonable justifications to just use it as-is.

> I don't see any reason this function should be written specific to
> libdir and bindir.  There are other file names that may usefully be
> relativized, so the function should be as general as possible, and use
> generic names, too.

  Sure; I just picked those names as indicative, not normative, examples of
usage.  How about I change them to destdir and srcdir?

>> +# call:
>> +#   func_dirname:
>> +#             func_dirname file append nondir_replacement
> 
> Why do you repeat the descriptions of these functions here again?
> That's not normally done elsewhere in the code (and anyway leads to
> unmaintainable text duplication).  Oh wait, it's done with
> func_dirname_and_basename, but only because the sole reason for that
> function is to wrap to other ones.

  Right, I was just copying that model.  If you want I'll happily snip it all
out.  Deleting stuff is easy!

> We also need to eventually give up the local variable name uglification
> in favor of something more readable.  This is already quite mad (but of
> course not your fault, nor reason to fix in this patch).

  Yeah, my first instinct was to use 'local', it's a real shame there's no
portable way to do that.

>> --- a/libltdl/config/ltmain.m4sh
>> +++ b/libltdl/config/ltmain.m4sh
>> @@ -1129,6 +1129,8 @@ The following components of LINK-COMMAND are treated 
>> specially:
>>  
>>    -all-static       do not do any dynamic linking at all
>>    -avoid-version    do not add a version suffix if possible
>> +  -bindir BINDIR    specify path to $prefix/bin (needed only when installing
>> +                    a Windows DLL)
> 
> Again, please don't give the impression that users should pass this for
> w32 only.

  Okeydokey, will snip the phrase in parentheses.

>> --- /dev/null
>> +++ b/tests/pe-dll-inst-bindir.at
> 
>> +noskip=:
>> +case "$host_os" in
>> +cygwin*|mingw*|cegcc*) ;;
>> +*) noskip=false ;;
>> +esac
>> +
>> +$noskip && {
> 
> You cannot wrap AT_SETUP/AT_CLEANUP blocks in shell conditionals.  It
> will mess up Autotest (the -l output, running specific tests only etc).
> Just consider any code outside of these blocks going to /dev/null.

  Ah.  Right, I was guessing there.  Since I've decided it makes sense to run
the tests on all platforms, the problem is moot.

>> +AT_BANNER([PE DLL install tests])
> 
> Please drop the banner, that's only really helpful for big sets of tests.

  Okeydokey.

>> +AT_SETUP([simple compile check])
> 
> I'd just compactify this into one test group; or name the groups
>   bindir compile, bindir basic, bindir install
> 
> or so.

  I'd like to take the second option, it'll go nicely with renaming the file
to bindir.at

>> +# Verify compiling works.
>> +
>> +AT_DATA([simple.c] ,[[
>> +int main() { return 0;}
>> +]])
>> +
>> +$noskip && {
>> +$CC $CPPFLAGS $CFLAGS -o simple simple.c 2>&1 > /dev/null || noskip=false
>> +rm -f simple 
> 
> Please avoid trailing white space.

  Oops, sorry!  I try to but must have missed that one.

> Your main doesn't need anything from libbar, and your libbar doesn't
> need anything from libfoo.  Why have the libraries in that case?

  I copied and pasted the testcase from elsewhere in the testsuite, without
trimming it any.

> Likewise in the test below.

  Well if I didn't have any library at all how could I check if it got
installed to the right place?  Anyway, this too was largely cut'n'pasted.
I'll trim them both down so that there's just one library and no main in each, 
OK?

> Using 'rm -rf /tmp/$$' is a straight path to a security CVE, and writing
> there still at least a DoS waiting to happen.  Don't ignore TMPDIR, and
> please use the sample mktemp replacement code from the Autoconf manual
> if you really need to create files in a temporary directory.

  I wanted to test the corner-case where the code in func_relative_path has to
ascend all the way to the root dir and make sure it breaks out of the loop
(rather than, say, looping forever), so I reckon I really do want to try it in
a temp dir.  I'll use $TMPDIR if set, default to /tmp if not, and make use of
the mktemp code you mentioned.

> What about duplicate slashes BTW?  They occur frequently with
>   --prefix=/some/where/
> or similar (new enough Autoconf releases strip the trailing slash in
> this case, so you might want to wait for my GCC autotools upgrade patch
> set).  But have you tested them?

  Yes, note how I test each possible bindir both with and without a trailing
slash, but I guess I should also do likewise for the libdir.  I deliberately
wrote the code to normalize all the paths on input and output to avoid this
problem - we're very aware of it on Cygwin because it's one of the few
platforms that actually takes advantage of the POSIX spec's leeway for a
leading "//" to mean something special.  I guess I should also add at least a
few tests that have multiple slashes on the end, or in the middle.

    cheers,
      DaveK





reply via email to

[Prev in Thread] Current Thread [Next in Thread]