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.
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.
On Wed, May 6, 2015 at 6:25 AM, Alon Bar-Lev <alon.bar...@gmail.com> 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.
> 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.
>
> Signed-off-by: Alon Bar-Lev <alon.bar...@gmail.com>
> ---
> 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.
> [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.
That said, the idea behind disabling the sysroot search just to
provide your own search path that does the same thing seems misguided.
> 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?
> #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.
> +clean-local:
> + rm -fr headers
Never call rm directly. There's an RM variable for this.
That said, why aren't you using the EXTRA* variables to do this?
------------------------------------------------------------------------------
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
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
