Bug 46175 - Full Mingw+MSys support
Summary: Full Mingw+MSys support
Status: RESOLVED FIXED
Alias: None
Product: APR
Classification: Unclassified
Component: APR (show other bugs)
Version: HEAD
Hardware: PC Windows XP
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Apache Portable Runtime bugs mailinglist
URL:
Keywords:
: 51369 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-11-10 02:08 UTC by Carlo Bramini
Modified: 2012-08-13 18:47 UTC (History)
3 users (show)



Attachments
Fix for this bug (16.20 KB, patch)
2008-11-10 02:08 UTC, Carlo Bramini
Details | Diff
Fix for this bug (6.33 KB, patch)
2011-02-04 14:38 UTC, Carlo Bramini
Details | Diff
Fix for this bug (4.62 KB, patch)
2011-03-29 14:07 UTC, Carlo Bramini
Details | Diff
Fix for this bug (8.87 KB, patch)
2011-03-30 12:31 UTC, Carlo Bramini
Details | Diff
Fix for this bug (4.26 KB, patch)
2011-03-31 15:01 UTC, Carlo Bramini
Details | Diff
Alternative fix for this bug. (3.21 KB, patch)
2011-04-01 10:37 UTC, Carlo Bramini
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlo Bramini 2008-11-10 02:08:09 UTC
Created attachment 22847 [details]
Fix for this bug

This patch adds complete support for mingw+msys.
Also tested with cygwin and Debian etch 4.0r4.
Comment 1 William A. Rowe Jr. 2008-11-10 12:46:50 UTC
Great stuff, thanks!  IF this fixes all issues, and none of the other open
cygwin/mingw bug patches are still applicable, it would be really cool if you
could close those with a pointer to this bug.

Reviewing to apply later in this week.  (search - commented by 'me' is how
I find stuff I need to follow up on).
Comment 2 William A. Rowe Jr. 2009-06-01 19:41:49 UTC
Sorry for not responding earlier.

This patch is frozen, primarily due to the number of unintended consequences
introduced by modification of apr.h.in across a broad number of platforms,
and the fact that the vast majority of changes to apr.h.in are not required
for the external consumers, nor the public interface.

Thus far; the tlhelp32.h should never have been included, and this has been
added for both to include/arch/win32/apr_arch_misc.h and removed from apr.hw.

Similarly other things should be clarified, and apr.h.in treated as the reference
file to *avoid* changing.  Other mistakes in the patch, such as;

 /* Mechanisms to properly type numeric literals */
+#ifdef _MSC_VER
+#define APR_INT64_C(val) (val##i64)
+#define APR_UINT64_C(val) (val##Ui64)
+#else
 @int64_literal@
 @uint64_literal@
+#endif

should have been corrected by adjusting [u]int64_literal definitions within
the autoconf script.

So this patch is in need of further explanation and clarification and absolutely
cannot be applied as-is to the stable shipping branch.

I'll try to piece through it, but in the future, consider submitting a number
of smaller single-function patches, so that the ones which are correct can be
applied immediately, while the ones which are incorrect can be clarified and
discussed.
Comment 3 John Vandenberg 2010-07-01 02:51:00 UTC
I see this patch adds -lmswsock.  I noticed that this was needed for bug 49535.  I didnt investigate why.

Also, this patch replaces APR_CHECK_DLL_FUNC with AC_LIBTOOL_WIN32_DLL; what is the minimum version of libtool where this works well?  That could mean a later version of autoconf is needed (currently APR depends on autoconf 2.59, which was released in Dec-2003).

If APR_CHECK_DLL_FUNC is being replaced, build/apr_win32.m4 should be deleted.

If mingw32 has sendfile, the configure script needs to find it.

I see you have removed APR_REMOVEFROM(CFLAGS,-O2)/APR_ADDTO(CFLAGS,-O0).  Does that mean your patch runs all tests with -O2.  That would be very good news, and I'd be keen to figure out what was the cause and help get the fix into trunk.
Comment 4 Zeno Davatz 2011-02-03 06:04:51 UTC
Hi

I am also interested in pushing this forward.

Best
Zeno
Comment 5 Carlo Bramini 2011-02-04 14:38:55 UTC
Created attachment 26607 [details]
Fix for this bug

After long long time I decided to do another try for improving the support for mingw+msys on APR.
All my changes were done against the most recent sources I could find into the SVN repository.
However, I must signal to you that I was not able to create configure script at first try, I had to process the following sources with dos2unix tool for changing all CRLF into LF, otherwise the creation failed:

buildconf
build/buildcheck.sh
build/PrintPath
build/get-version.sh

After bypassing this little trouble, I was able to start my inspection.
As you can see, I trashed all previous fixes I did for BeOS since at current time I could not test them anymore.
I also trashed the fixes to APR_INT64_C and APR_UINT64_C for MSVC.
In the past I did that change because I though that it could be useful to use directly the apr.h file generated from configure script with mingw in VisualStudio, perhaps with just some little eventual adjustment if some libraries are available in mingw at configure time while they are not in MSVC.
In other words, my intention was to create a facility for the maintenance of apr.hw (you can also notice that I had modified a bit apr.hw to be very similar to generated apr.h for simplify file comparison), which could be updated with a simple copy paste action.
Since apr.h.in already included some tests for __GNUC__, I though it would not be a big problem to add one test for _MSC_VER in that point.

My fixes to current trunk can be resumed as:

1) Build process was not able to create a shared library.
For creating a DLL, libtool requires the -no-undefined flag al linking stage. This flag has been added in $LT_LDFLAGS macro.
Unfortunately, in some places the $LT_LDFLAGS was placed wrongly before calling the driver, in other words was at the same level of macro LTFLAGS and as result those options were given direclt to libtool rather than the driver.
I fixed that mistake in these files:
test/Makefile.in
configure.in

2) I was a bit out of idea when I tried to understand the reasons for testing shell32, advapi32, ws2_32 and rpcrt4 in that manner in configure script.
Since these tests where done in the mingw section, that reason is still arcane to me...
Another thing that should be avoided is to add -lkernel32 in the command line: for some reasons, mingw hates this and anyways you do not need to write them as well as user32 and msvcrt: you will have them by default.
If you had to add them previously, probably your compiler or at least your SPEC file is very outdated and so the best solution is to update it.

3) I modified a piece of hand written code for using APR_SETIFNULL instead. I also added declaration of some variables that may be left empty regarding the platform.

4) I'm able to compile APR without problems with -O2 flag.
I'm using the stable gcc-3.4.5-20060117-3.
Experimental gcc-4.x probably will work fine too, but I suggest to use the stable version since it's really stable in its word sense and with my experience it seems to me it is able to generate more efficient code.
If you are using the cross compiler that you may find in a linux distrubution, you can encounter problems because some bugs in the toolchain: when working for building a native port of GUILE for Windows, some people noticed that it was not able to export data from DLL but only functions, while I was perfectly able to build it.
It may be possible that the compiler used by somebody in the past had a similar defect.
If you are working on that bugged cross compiler, the only solution is to replace it with a working one. You may compile it yourself with the sources of the mingw stable (that I recommend) or other precompiled binaries, like the ones provided by mingw-w64 or the ones included into the RosBE enviroment for linux that we use for compiling ReactOS.
Another alternative that could cause that old trouble at linking could be a mistake in support macro for forcing the usage of shared runtime (missing _DLL, __MSVCRT__ and so on...) but since there are not informations about, it's difficult to make more theories...

5) support/unix/waitio.c failed to compile.
Since the pollset member of apr_file_t structure is made conditional to APR_FILES_AS_SOCKETS value, I simply added its test.



PROBLEMS:
While I was able to compile and install APR, I got some new problems.

The first problem happened because I installed sqlite3 in my build enviroment.
During configuration, sqlite3 has been detected correctly and enabled.
But when linking the DLL, it showed an error saying that apr_dbd_sqlite3_driver was undefined.
I do not know what's wrong exactly, but the only solution was to disable sqlite3 support.

The creation of the DLL also failed because it was not adding iconv when linking and infact it complained about unresolved libiconv symbols.
Actually I fixed it directly in the generated makefile by manually add "-liconv" but it must be fixed correctly.
Probably a more cleaner solution exists and perhaps it would be better to trash all that current code and simply use AM_ICONV for detecting this stuff.


NOTES:
AC_LIBTOOL_WIN32_DLL is (was) a macro for activating the support for the creation of dynamic linking libraries in Windows.
Although it's deprecated (there is a new technique, as it has also been written in your configure.in), it will be maintained for compatibility with all future scripts, at least this is what I remember I read.
But I do not see much relation with your APR_CHECK_DLL_FUNC macro.
Anyways I think it could be removed since it's unused if you will apply my patch.

Sincerely,

Carlo Bramini.
Comment 6 Zeno Davatz 2011-02-08 05:23:45 UTC
Hi

I applied dos2unix to the files as mentioned above. But I still get the following errors when doing 

C:\Users\zdavatz\software\apr>sh buildconf
buildconf: checking installation...
buildconf: python version 2.5 (ok)
buildconf: autoconf version 2.67 (ok)
buildconf: libtool version 2.4 (ok)
buildconf: copying libtool helper files using /c/MinGW/bin/libtoolize
buildconf: creating include/arch/unix/apr_private.h.in ...
autom4te-2.67: m4sugar/m4sugar.m4: no such file or directory
autoheader-2.67: failed to run '/c/MinGW/bin/autom4te-2.67': Bad file number
buildconf: creating configure ...
autom4te-2.67: m4sugar/m4sugar.m4: no such file or directory
buildconf: generating 'make' outputs ...
buildconf: rebuilding rpm spec file

on the way I have to kill mktemp.exe 3 times.

Best
Zeno
Comment 7 Carlo Bramini 2011-02-10 07:24:21 UTC
I have not that problem... tested with msys and cygwin too.
The "missing" file is actually provided by autoconf-2.67 package and I have it here:
/share/autoconf/m4sugar/m4sugar.m4
or here (which is just an alias of previous path):
/usr/share/autoconf/m4sugar/m4sugar.m4

By reading your message, it looks like you are mixing mingw tools and msys tools together, this is not exactly a lighted idea since I remember you can encounter problems with path interpretation and other things too.
In my enviroment, I put all mingw stuff in c:\mingw and all msys stuff in c:\msys and everything always worked fine.
I could try to do the same thing.
Comment 8 Zeno Davatz 2011-02-10 07:33:22 UTC
Thanks, I solved that bit with some help of Günter. My msys came after my Git-Msys in my Path so that was the problem. Configure now runs, but make does not.

Please also see this message from Günter: 

http://www.mail-archive.com/dev@apr.apache.org/msg23583.html

best
Zeno
Comment 9 Jeff Trawick 2011-03-18 16:29:40 UTC
a very tiny bit was committed with 

http://svn.apache.org/viewvc?rev=1083038&view=rev

sorry it wasn't more; I spent most of my available time getting a build environment  (wasn't bad at all, but the clock hands did spin; more progress next time)
Comment 10 Jeff Trawick 2011-03-19 14:07:55 UTC
more committed with
http://svn.apache.org/viewvc?rev=1083242&view=rev
Comment 11 Jeff Trawick 2011-03-19 14:31:27 UTC
trying this part of the patch (below), I get this failure in the test dir:

.libs/globalmutexchild.o: In function `main':
c:\Users\Trawick\svn\apr-xx\test/globalmutexchild.c:38: undefined reference to `
apr_initialize'
c:\Users\Trawick\svn\apr-xx\test/globalmutexchild.c:39: undefined reference to `
apr_terminate'
(and so on)

Index: test/Makefile.in
===================================================================
--- test/Makefile.in	(revision 1083243)
+++ test/Makefile.in	(working copy)
@@ -74,7 +74,7 @@
 
 # link programs using -no-install to get real executables not
 # libtool wrapper scripts which link an executable when first run.
-LINK_PROG = $(LIBTOOL) $(LTFLAGS) --mode=link $(LT_LDFLAGS) $(COMPILE) \
+LINK_PROG = $(LIBTOOL) $(LTFLAGS) --mode=link $(COMPILE) $(LT_LDFLAGS) \
 	    @LT_NO_INSTALL@ $(ALL_LDFLAGS) -o $@
 
 # STDTEST_PORTABLE;
Index: configure.in
===================================================================
--- configure.in	(revision 1083243)
+++ configure.in	(working copy)
@@ -292,7 +292,7 @@
 if test "x$use_libtool" = "xyes"; then
       lt_compile='$(LIBTOOL) $(LTFLAGS) --mode=compile $(COMPILE) -o $@ -c $< && touch $@'
       LT_VERSION="-version-info `$get_version libtool $version_hdr APR`"
-      link="\$(LIBTOOL) \$(LTFLAGS) --mode=link \$(LT_LDFLAGS) \$(COMPILE) \$(LT_VERSION) \$(ALL_LDFLAGS) -o \$@"
+      link="\$(LIBTOOL) \$(LTFLAGS) --mode=link \$(COMPILE) \$(LT_LDFLAGS) \$(LT_VERSION) \$(ALL_LDFLAGS) -o \$@"
       so_ext='lo'
       lib_target='-rpath $(libdir) $(OBJECTS)'
       export_lib_target='-rpath \$(libdir) \$(OBJECTS)'
@@ -308,6 +308,9 @@
     *-solaris2*)
         apr_platform_runtime_link_flag="-R"
         ;;
+    *-mingw*)
+        LT_LDFLAGS="$LT_LDFLAGS -no-undefined"
+        ;;
     *)
         ;;
 esac
Comment 12 Carlo Bramini 2011-03-19 21:02:18 UTC
I confirm that the compilation fails when launching "make test".
Anyways, the problem is not related the last patch you applied (that fix is correct), it happens because into apr.h.in is missing the handling of APR_DECLARE_EXPORT and APR_DECLARE_STATIC for generating correct macro for:

APR_DECLARE(type)       
APR_DECLARE_NONSTD(type)
APR_DECLARE_DATA        

At the moment only APR_MODULE_DECLARE_DATA is handled correctly in apr.h.in as it has been done in apr.hw (and infact, if you check the content of libapr-0.dll you will just find some exported data).
As you can see the solution is quite easy, all depends if a change will be allowed to apr.h.in or not, otherwise it seems to me that you will be able to use APR only if configure is launched with "--enable-static --disable-shared".
Of course, configure script also need to handle APR_DECLARE_EXPORT and APR_DECLARE_STATIC somewhere, to make it working, depending on the above options given for libtool.
Comment 13 Carlo Bramini 2011-03-29 14:07:55 UTC
Created attachment 26812 [details]
Fix for this bug

This new patch includes the fixes of previous one (excluding the ones that you have already committed) and it includes these news:

test/Makefile.in:
mod_test depends on libapr-2 so I must add its reference when linking.

include/apr.h.in:
Copied the same code written into apr.hw for handling these macro:
APR_DECLARE
APR_DECLARE_NONSTD
APR_DECLARE_DATA
Actually, I could not see many other ways for doing it...
I did my best to think something that could be handled by configure itself, but the fact to have different declarations when building the library itself (read: dllexport) and derived code (read: dllimport) limited a lot the alternatives.
Perhaps it would not be a big problem having these macros fixed here because it seems they have some sense only when building for Windows and they are technically useless on other enviroments.

configure.in:
Added detection of build type, if user configured libtool for static or shared build, APR_DECLARE_STATIC or APR_DECLARE_EXPORT will be defined.

xml/apr_xml_internal.h:
Declaration of apr_xml_parser_create_ex was bugged because it was not prefixed by APR_DECLARE and it was not exported as expected. Now it's fixed.


I configure APR with:

../apr-svn/configure --disable-static --enable-shared --prefix=$HOME/inst_apr --with-sqlite3=no --with-iconv=no

and with:

../apr-svn/configure --enable-static --disable-shared --prefix=$HOME/inst_apr --with-sqlite3=no --with-iconv=no

and compilation was completed successfully on both.
Now, "make test" works, but I'm getting these results at the end:


Failed Tests            Total   Fail    Failed %
===================================================
testdso                     5      4     80.00%
testfile                   36      1      2.78%
testpipe                   10      1     10.00%
testsock                   10      1     10.00%
testpass                    4      2     50.00%


I'm getting the same exact report for both static and shared build.
I do not know if Windows platform dependant sources have been ever tested by this testsuite.
Comment 14 Jeff Trawick 2011-03-29 18:32:54 UTC
The correction to the linkage of apr_xml_parser_create_ex() was checked in as 1086790.

Your testall output looks vaguely like I saw recently with a Microsoft toolchain build, but now that build is broken so I can't confirm.

(more eyes on the rest of the patch soon)
Comment 15 Carlo Bramini 2011-03-30 12:31:27 UTC
Created attachment 26817 [details]
Fix for this bug

I did a quick check on the failure that happens in testfile entry.
This failure happens in test_file_trunc() and it fails to truncate the file when it is open with APR_FOPEN_BUFFERED flag.
I think there is also a bug in use of SetFilePointer API.
Instead of fixing all places where it is called, I made an utility function "SetFilePointerApr" that should handle the task correctly and it simplifies a lot the code too.

I also did a quick compilation on CygWin with latest APR sources and I updated the patch a bit with its fixes too.

build/apr_hints.m4:
Do not add an useless macro, if you want to test CygWin at compile time, simply use __CYGWIN__

file_io/unix/open.c:
There is a bug into apr_file_open(): 'rv' is used also in line 244, near apr_file_info_get(), so declaring it inside an #if APR_HAS_THREADS ... #endif made compilation to fail if APR_HAS_THREADS=0.

file_io/win32/seek.c
Fixed usage of SetFilePointer() and truncation of the file.



After launching "make test" on CygWin, I got these result:

============

This program won't work on this platform because there is no support for threads
.
This test requires APR thread support.
testatomic          : SUCCESS
testdir             : SUCCESS
testdso             : FAILED 8 of 9
testdup             : SUCCESS
testenv             : SUCCESS
testfile            : SUCCESS
testfilecopy        : SUCCESS
testfileinfo        : SUCCESS
testflock           : SUCCESS
testfmt             : SUCCESS
testfnmatch         : SUCCESS
testargs            : SUCCESS
testhash            : SUCCESS
testhooks           : SUCCESS
testipsub           : FAILED 1 of 5
testlock            : SUCCESS
testcond            : SUCCESS
testlfs             : SUCCESS
testmmap            : SUCCESS
testnames           : SUCCESS
testoc              : SUCCESS
testpath            : SUCCESS
testpipe            : SUCCESS
testpoll            : SUCCESS
testpools           : SUCCESS
testproc            : |proc_child:./.libs/lt-proc_child.c:233: FATAL: couldn't f
ind proc_child.
FAILED 2 of 3
testprocmutex       : -/bin/sh: line 2:  2860 Bad system call         PATH="`ech
o "../dbm/.libs:../dbd/.libs:../ldap/.libs:$PATH" | sed -e 's/::*$//'`" ./$prog
Usage: /home/Carlo/apr/test/./.libs/dbd driver-name [params]
$
make[1]: *** [check] Error 140
make[1]: Leaving directory `/home/Carlo/apr/test'
make: *** [check] Error 2


Unfortunately, I do not know exactly what happens at the end.
I also do not understand the sentence "no support for threads".
Comment 16 William A. Rowe Jr. 2011-03-30 13:19:11 UTC
Carlo,

your report...

"I did a quick check on the failure that happens in testfile entry.
This failure happens in test_file_trunc() and it fails to truncate the file
when it is open with APR_FOPEN_BUFFERED flag.
I think there is also a bug in use of SetFilePointer API.
Instead of fixing all places where it is called, I made an utility function
"SetFilePointerApr" that should handle the task correctly and it simplifies a
lot the code too.

is a pretty serious bug.

However, it has nothing to do with the subject of this incident.  Modifying
this patch over and over to fix *unrelated bugs* makes the changes very hard
to follow, puts all of the work on the one developer who's patiently been
going through the suggested mingw/msys changes, depriving him of his time and
depriving the rest of the developers the chance to review this specific bug.

Can you please stay focused on "these requirements to build using msys/mingw"
on this incident, and create new incidents when you spot other bugs, please?
Comment 17 Jeff Trawick 2011-03-30 13:35:56 UTC
Hi Carlo, 

First, thanks for all your efforts.  I was about to say approx. the same thing as Bill :)

I will go ahead and handle the trivial variable declaration issue in unix/open.c but not attribute it to any particular bug.

Open a bug (or post patch + commentary to dev@apr) for just the file ptr issue.

Open another bug (or post patch + comm...) for just the Cygwin build issue(s).

Can you split the remaining MinGW stuff up to by issue (perhaps 2-3 issues in there)?

As far as this bug...  Much of the reported issues are now resolved in 1.4.x through trunk now and it is seemingly usable, so I think we should close this one out and add a note in 1.4.x/CHANGES indicating that we have experimental MinGW/MSys support.  I think the word "experimental" is important because until we have at least the feature selection (e.g., APR_HAVE_IPV6 and the like) fixed, we can't promise an equivalent build for MinGW when replacing 1.4.x with 1.4.y and using the same build commands.

Concerns?
Comment 18 Carlo Bramini 2011-03-30 18:30:53 UTC
Hello friends,
thank you very much for your replies.
Please excuse me if I made some confusion with patches, I was thinking that it could be useful to have all fixes for compiling APR with autotools on Windows in a single patch.
Evidently, I got the bad idea when I estimated those fixes to be ridiculously small.
But hopefully, this is also the easiest thing to fix :D
Actually the only remaining serious problem to overcome for closing this bug is the limited set of fixes for generating a shared library.
If it's ok for you, I will do a smaller patch that you could review, for fixing just this defect, hopefully there is not really much work to do before reaching this target.
Then I will create two different bugs for proposing a fix to the problem I think I have found in file truncation and for removing a macro defined for CygWin that is totally redundant (and also unused).
Sorry for the trouble...

Sincerely,

Carlo Bramini.
Comment 19 Jeff Trawick 2011-03-30 19:23:43 UTC
sure, getting the shared library support working as one last patch for this bug works for me

take care!
Comment 20 Carlo Bramini 2011-03-31 15:01:21 UTC
Created attachment 26821 [details]
Fix for this bug

This last patch should fix creation of shared libraries (DLL) under MinGW+MSys.
The corrections can be resumed in:

test/Makefile.in:
* $(LT_LDFLAGS) must be placed after the driver, otherwise it will act exactly like $(LTFLAGS). This is required, otherwise libtool will be never able to recognize the "-no-undefined" flag.
* if mod_test.la does not specify $(LOCAL_LIBS), compilation will fail (this is required by both static and shared builds).

include/apr.h.in:
* It is required to declare APR_DECLARE, APR_DECLARE_NONSTD and APR_DECLARE_DATA as they are in apr.hw, otherwise the generate DLL won't export any symbol. APR_MODULE_DECLARE_DATA is already included, but previous ones are not.

configure.in:
* $(LT_LDFLAGS) must be placed after the driver, as described for test/Makefile.in.
* Added "-no-undefined" flag to $(LT_LDFLAGS) if the target platform is Windows.
* Declare APR_DECLARE_EXPORT or APR_DECLARE_STATIC, required by the macro added in include/apr.h.in.
* Removed the "strange" libraries detection (btw, probably it was also breaking platforms without __stdcall calling convention, like Windows CE/Mobile/Phone, since function names were decorated), linker gave error without sense if adding "-lkernel32", probably because it creates a conflict of library precedence and dependency.

That's all, all these topic have also a good description in previous posts, if you think that something else should be adjusted in this patch, let me know!

Sincerely,

Carlo Bramini.
Comment 21 Jeff Trawick 2011-03-31 20:44:07 UTC
About the use of APR_DECLARE_STATIC/APR_DECLARE_EXPORT:

(And yes, I probably am confused; just tell me where.)

The Microsoft toolchain build is set up to be able to generate statically- and dynamically-linked versions of APR which can coexist in the APR installation directory, and the user defines one of those symbols when building their application so that the one copy of the include files declare the correct linkage depending on whether the application will be statically or dynamically linked with APR.  (Yeah, they're also used in the APR build itself since there's one apr.h for both builds.)

The autoconf-based toolchain can only generate static or dynamic in one build, and they apparently cannot coexist.  A couple of examples:

1. There can only be one apr-2-config, and "apr-2-config --cflags" will have either APR_DECLARE_STATIC or APR_DECLARE_EXPORT, depending on the last one which was installed.  (That could be ripped out of apr-2-config, and maybe Windows users won't make use of that anyway.)

2. (Fuzzy recollection, unfortunately) I installed a dynamic build then a static build into the same directory; when I linked my app, the link step would reference the apr DLL if libapr-2.dll.la existed in the INSTALLDIR/lib directory; when the la was removed it would statically link with apr.

If the coexistence of static/dynamic is not practical with the autoconf-based build, I don't think we need to expose the APR_DECLARE_STATIC/APR_DECLARE_EXPORT "choice" for that build.  IOW, don't add it to CPPFLAGS, and substitute the proper linkage when apr.h is generated ("#define APR_DECLARE(type)  @aprdcl@" and the like) and there won't be multiple versions in the final apr.h.

Thoughts?

BTW, the patch has three versions of the declare macros:

if APR_DECLARE_STATIC
  foo
else if APR_DECLARE_EXPORT
  bar
else
  baz
endif

What is that default set of definitions used for?  I thought one of those APR_DECLARE_ would always be defined.
Comment 22 William A. Rowe Jr. 2011-03-31 23:12:16 UTC
APR_DECLARE_STATIC - undecorated normal linkage

APR_DECLARE_EXPORT - INTERNAL apr build mechanic

no defines - external apr library linkage

Most .dll consumers need to define nothing, while .lib users aught to define
APR_DECLARE_STATIC.
Comment 23 Jeff Trawick 2011-04-01 07:17:37 UTC
Thanks, Bill.  I can't read the README or the code properly :(

That shows why two flavors of the declarations are required for dynamic builds -- one for internal, one for external.  (We would still have the dynamic and static support in apr.h, as if the two installs could coexist, but that's a minor detail.)

APR_DECLARE_EXPORT definitely can't leak outside of the APR build, independent of other issues.  Right now it shows up in "apr-2-config --cppflags".
Comment 24 Jeff Trawick 2011-04-01 09:25:23 UTC
BTW, I plan to commit the current patch, close this bug, and open another for the apr-2-config glitch (exposing the EXPORT in app CPPFLAGS).

Co-resident static and dynamic installs are yet another potential concern (not to me, as long as we document it ;) ).
Comment 25 Carlo Bramini 2011-04-01 09:34:12 UTC
You are right, this fact escaped to my attention.
The presence of "-DAPR_DECLARE_EXPORT" into apr-2-config is a damage because the declaration of public functions will be always prefixed by __dllexport rather than __dllimport.
Instead, "-DAPR_DECLARE_STATIC" is required for a static build even by client applications.
In my opinion the solution is to separate the flags used by compilation of APR itself from the ones used by client applications:
static build: APR_DECLARE_STATIC for both.
shared build: APR_DECLARE_EXPORT when compiling APR and no additional macros for client apps.

Is there already this chance available in APR building system?

There is also another alternative, that could automatically solve the trouble, at least in theory: remove all APR_DECLARE_STATIC and APR_DECLARE_EXPORT additions (and also APU_MODULE_DECLARE_STATIC since we are fixing) to apr.h.in, remove the same addition in configure.in and just tell to libtool to export all symbols, we just need to add an options to its flags.

I think that the second choice would be better, if things are getting too complicated.
Comment 26 Carlo Bramini 2011-04-01 10:37:36 UTC
Created attachment 26832 [details]
Alternative fix for this bug.
Comment 27 Jeff Trawick 2011-04-01 11:28:04 UTC
This seems to work for creating CPPFLAGS which are used only for the build of APR itself:

Index: configure.in
===================================================================
--- configure.in	(revision 1087516)
+++ configure.in	(working copy)
@@ -2668,6 +2668,7 @@
 
 dnl ----------------------------- Construct the files
 
+AC_SUBST(INTERNAL_CPPFLAGS)
 AC_SUBST(LDLIBS)
 AC_SUBST(INCLUDES)
 AC_SUBST(AR)
Index: build/apr_hints.m4
===================================================================
--- build/apr_hints.m4	(revision 1087516)
+++ build/apr_hints.m4	(working copy)
@@ -119,6 +119,7 @@
 	APR_ADDTO(CPPFLAGS, [-DHPUX -D_REENTRANT])
 	;;
     *-linux*)
+        APR_ADDTO(INTERNAL_CPPFLAGS, -DJEFF)
         case `uname -r` in
 	    2.* )  APR_ADDTO(CPPFLAGS, [-DLINUX=2])
 	           ;;
Index: build/apr_rules.mk.in
===================================================================
--- build/apr_rules.mk.in	(revision 1087516)
+++ build/apr_rules.mk.in	(working copy)
@@ -55,6 +55,10 @@
 EXTRA_LIBS=@EXTRA_LIBS@
 EXTRA_INCLUDES=@EXTRA_INCLUDES@
 
+# CPPFLAGS which are valid only while building APR itself
+#
+INTERNAL_CPPFLAGS=@INTERNAL_CPPFLAGS@
+
 # NOTEST_* are flags and libraries that can be added by the user without
 # causing them to be used in configure tests (necessary for things like
 # -Werror and other strict warnings that maintainers like to use).
@@ -70,7 +74,7 @@
 # left-to-right precedence and CPPFLAGS may include user-defined overrides.
 #
 ALL_CFLAGS   = $(EXTRA_CFLAGS) $(NOTEST_CFLAGS) $(CFLAGS)
-ALL_CPPFLAGS = $(DEFS) $(EXTRA_CPPFLAGS) $(NOTEST_CPPFLAGS) $(CPPFLAGS)
+ALL_CPPFLAGS = $(DEFS) $(INTERNAL_CPPFLAGS) $(EXTRA_CPPFLAGS) $(NOTEST_CPPFLAGS) $(CPPFLAGS)
 ALL_LDFLAGS  = $(EXTRA_LDFLAGS) $(NOTEST_LDFLAGS) $(LDFLAGS)
 ALL_LIBS     = $(LIBS) $(NOTEST_LIBS) $(EXTRA_LIBS)
 ALL_INCLUDES = $(INCLUDES) $(EXTRA_INCLUDES)
Comment 28 William A. Rowe Jr. 2011-04-01 16:00:36 UTC
Two installs -can- coexist rather easily.  (.so's alongside .a's, and they
should likely be named libapr-2.dll.a vs libapr-2.a)  But -building- them
side by side, I'd presume the .o targets would clash.
Comment 29 Jeff Trawick 2011-04-01 16:24:26 UTC
Building them side by side wasn't an issue (make extraclean or presumably VPATH).  The issues I noted were with coexisting installs.

Carlo, what about the lack of __stdcall vs. __cdecl, dllexport vs. dllimport vs. nothing in your latest patch?
Comment 30 William A. Rowe Jr. 2011-04-01 17:19:44 UTC
Note that the __stdcall semantic is absolutely required if we are going to be
able to build a binary targeting libapr-2.dll (irrespective of whether it came
from a msvc or mingw build).
Comment 31 Jeff Trawick 2011-04-02 08:07:31 UTC
I think I just take Carlo's patch n-1, add the trick for internal-only CPPFLAGS in order to avoid the dllimport vs. dllexport issue in that patch, and the linkage is resolved.
Comment 32 Jeff Trawick 2011-04-02 09:51:05 UTC
thanks all!
(r1088023)
Comment 33 Jeff Trawick 2011-06-13 23:17:17 UTC
*** Bug 51369 has been marked as a duplicate of this bug. ***
Comment 34 Jeff Trawick 2012-08-12 22:56:41 UTC
This has been merged to the 1.5.x branch for apr future 1.5.0.
(Thanks again!)
Comment 35 Jeff Trawick 2012-08-13 18:47:20 UTC
A few pieces had to be committed to apr-util 1.5.x.  They're ready for upcoming apr-util 1.5.1.

http://svn.apache.org/viewvc?view=revision&revision=1372546