On 6 May 2015 at 16:09, NightStrike <[email protected]> wrote: > > I know I've been silent for a long time, but I do still read the > lists, and I felt the need to comment on what I consider a bad change. > Feel free to ignore me.
thank you for your comments. > I understand the intent, and the intent is good (that is, to make top > level work again). I still have some local patches to that effect > from before the git switch that I never committed in time, and I won't > be using git anytime soon, so it's good that someone else is doing it. > But, doing it poorly is worse than not at all. please feel free to send these patches so I can see if I can improve these. > > On Wed, May 6, 2015 at 6:25 AM, Alon Bar-Lev <[email protected]> wrote: > > this effects only top level package, if crt is to be built: > > 1. install headers into builddir for staging > > 2. adjust CPPFLAGS for tools and libraries to find local headers > > This is a very bad idea. You are never allowed to modify the CPPFLAGS > variable. That is a user only variable, not for use by the package. appending (not replacing) to CPPFLAGS or CFLAGS or LDFLAGS within autoconf is widely used, I never saw that comment. I can remove it in favour of yet another substitution. I do not see the benefit, nor do I see such restriction in the autoconf manual. > > 3. disable sysroot for crt so search path contain only local headers > > This makes no sense. Just change the sysroot that you are passing in. there is no need to do so, as the CPPFLAGS are enough and respected by the crt, the default of having sysroot as $prefix when sysroot is something that may be considered to change, default should probably be no. > > > > Signed-off-by: Alon Bar-Lev <[email protected]> > > --- > > Makefile.am | 2 +- > > configure.ac | 10 ++++++++-- > > mingw-w64-crt/configure.ac | 5 +++-- > > mingw-w64-headers-local/Makefile.am | 9 +++++++++ > > 4 files changed, 21 insertions(+), 5 deletions(-) > > create mode 100644 mingw-w64-headers-local/Makefile.am > > > > diff --git a/Makefile.am b/Makefile.am > > index 26a7606..0cf559c 100644 > > --- a/Makefile.am > > +++ b/Makefile.am > > @@ -3,7 +3,7 @@ if HEADER > > endif > > > > if CRT > > - MAYBE_CRT = mingw-w64-crt > > + MAYBE_CRT = mingw-w64-headers-local mingw-w64-crt > > endif > > > > if LIBRARIES_MANGLE > > diff --git a/configure.ac b/configure.ac > > index 4bb3926..e0f7685 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -44,7 +44,13 @@ AC_ARG_WITH([crt], > > [], > > [with_crt=yes]) > > AS_CASE([$with_crt], > > - [yes],[AC_CONFIG_SUBDIRS([mingw-w64-crt])], > > + [yes],[ > > + AC_CONFIG_SUBDIRS([mingw-w64-crt]) > > + > > CPPFLAGS="-I\$(top_builddir)/../mingw-w64-headers-local/headers/include > > ${CPPFLAGS}" > > + export CPPFLAGS # so will effect subpackages > > + SYSROOT_DISABLE=yes > > + export SYSROOT_DISABLE # so will effect crt > > + ], > > You are breaking formatting conventions established in all of the > other files. You don't need two lines to set and export a variable. > You should not be writing this raw shell code anyway, as autoconf > gives you facilities to both set and propagate variables in a > portable, maintainable way. ok, will use AS_VAR_SET, but I still need to export, right? I do not know if there is a way to do this using AS_* macros. > > > [no],[], > > [MW64_OPTION_ERROR([with-crt])]) > > AM_CONDITIONAL([CRT],[test "x$with_crt" = xyes]) > > @@ -108,6 +114,6 @@ > > AM_COND_IF([TOOLS_GENDEF],[AC_CONFIG_SUBDIRS([mingw-w64-tools/gendef])]) > > AM_COND_IF([TOOLS_GENIDL],[AC_CONFIG_SUBDIRS([mingw-w64-tools/genidl])]) > > AC_MSG_RESULT([$with_tools]) > > > > -AC_CONFIG_FILES([Makefile]) > > +AC_CONFIG_FILES([Makefile mingw-w64-headers-local/Makefile]) > > AC_OUTPUT > > > > diff --git a/mingw-w64-crt/configure.ac b/mingw-w64-crt/configure.ac > > index bcf7d0c..387380f 100644 > > --- a/mingw-w64-crt/configure.ac > > +++ b/mingw-w64-crt/configure.ac > > @@ -18,7 +18,8 @@ AC_ARG_WITH([sysroot], > > [AS_HELP_STRING([--with-sysroot=DIR], > > [Search for headers within DIR/include (default: prefix)])], > > [], > > - [AS_VAR_SET([with_sysroot],[yes])]) > > + [AS_VAR_IF([SYSROOT_DISABLE], [yes], > > + [AS_VAR_SET([with_sysroot],[no]], > > [AS_VAR_SET([with_sysroot],[yes])]))]) > > Generally speaking, convention is to use AS_CASE whenever possible. > The resulting shell code is faster and more portable. It also is > easier to maintain, as the expressiveness is more powerful without > being complex. ok, will change. > That said, the idea behind disabling the sysroot search just to > provide your own search path that does the same thing seems misguided. the same method is applies to all crt, libraries and tools, I can probably work out something special for crt, but I think that the less logic we have the better. > > AS_CASE([$with_sysroot], > > [no],[], > > [yes],[AS_VAR_SET([with_sysroot],[$prefix])], > > @@ -252,7 +253,7 @@ AC_MSG_RESULT([$enable_tests_unicode]) > > #AC_FUNC_VPRINTF > > #AC_CHECK_FUNCS([alarm atexit btowc fesetround floor ftruncate > > gettimeofday isascii localeconv mbrlen memmove memset pow rint setlocale > > sqrt strcasecmp strchr strncasecmp strtoull strtoumax]) > > > > -AC_CHECK_HEADER([_mingw_mac.h], [], [AC_MSG_ERROR([Please check if the > > mingw-w64 header set and the build/host option are set properly.])]) > > +AC_CHECK_HEADER([_mingw_mac.h], [], [AC_MSG_WARN([Please check if the > > mingw-w64 header set and the build/host option are set properly.])]) > > Why are you changing this to a warn instead of error? because at the time of configure the mingw-w64-headers-local has not been built yet, so include files cannot be located as not built yet. I can add an environment variable to disable error mode if you think it should remain error. > > #Warnings and errors, default level is 3 > > AC_MSG_CHECKING([for warning levels]) > > diff --git a/mingw-w64-headers-local/Makefile.am > > b/mingw-w64-headers-local/Makefile.am > > new file mode 100644 > > index 0000000..9047e8b > > --- /dev/null > > +++ b/mingw-w64-headers-local/Makefile.am > > @@ -0,0 +1,9 @@ > > +all-local: clean-local > > + $(MAKE) \ > > + -C "$(top_builddir)/mingw-w64-headers" \ > > + prefix="$(abs_builddir)/headers" \ > > + includedir="$(abs_builddir)/headers/include" \ > > + libdir="$(abs_builddir)/headers/lib" \ > > + install > > Making all depend on clean is a terrible thing to do. No user will > ever, ever expect for any package build that doing a "make" will > delete a significant portion of the build directory. This is a very > evil thing to do. This should absolutely not ever be committed > anywhere ever for any project. You really need to read the guidelines > in the make manual. I do aware of these conventions, and in this case nothing significant is deleted, you do understand that the mingw-w64-headers-local is just a stub, it does not actually build anything, right? The alternative was: all-local: rm -fr headers $(MAKE) ... clean-local: rm -fr headers both should remove the headers, thus I wanted to remove the duplication. but if you think this duplication is better, then I will introduce that. your comment about this is pretty harsh... please understand that the mingw-w64-headers-local actually does nothing but make install of headers. > > > +clean-local: > > + rm -fr headers > > Never call rm directly. There's an RM variable for this I am unaware of this RM, autoconf uses rm all over. it is not like MKDIR_P. do you suggest I probe for rm using AC_CHECK_PROG? > That said, why aren't you using the EXTRA* variables to do this? please help me to understand how. Thanks, Alon ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y _______________________________________________ Mingw-w64-public mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
