This is the mail archive of the crossgcc@sourceware.org mailing list for the crossgcc project.

See crosstool-NG for lots more information.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] kernel/mingw64: add mingw64 kernel type


Yann, All,

Thanks for posting this again.

Overall, the patch looks OK. It built fine on my machine. Yeah! :-)

However, I have a few comments:

 1- I'd like to see how hard it would be to commonalise the mingw{32,64}
    variants together, eg. take advantage on CT_ARCH_64 to decide what
    mingw variant to build.
    This would avoid duplicating code path for mingw.

 2- In your patch, the DirectX and DDK options are in the mingw64 C library
    menu, but are used in the mingw64 kernel build script. That's wrong.
    If the option is used in the kernel script, it shall be in the kernel
    menu, not the C library menu.

 3- Also, this is incoherent with the 32-bit variant, for which the DirectX
    option *is* in the C library menu. I'd like that the two variants have
    a similar layout in the menu, so that it does not erquire a completely
    schyzophrenic user to discover where the options are. ;-)
    I see that for the 64-bit variant, the DirectX is provided by the kernel
    headers, whereas it is provided by a separate package and build in the
    libc_finish step for the 32-bit variant. If we have to change the 32-bit
    variant so it more closely ressemble the 64-bit one, so be it.
    That will help us commonalise the build scripts and the config files.

 4- Other comments in-lined in the patch, below.

On Tuesday 23 October 2012 Yann Diorcet wrote:
> # HG changeset patch
> # User Yann Diorcet <diorcet.yann@gmail.com>
> # Date 1351016048 -7200
> # Node ID a34dced6ae17acc54d132e91805208971e6ac7f4
> # Parent  946d6d133a90935465e3582b54b60c0d7e4e6397
> kernel/mingw64: add mingw64 kernel type
> script: add script for kernel and libc for mingw64
> samples: add 64 bits Windows target sample
> Signed-off-by: Yann Diorcet <diorcet.yann@gmail.com>

As this is primarily adding a new 'kernel', I'd suggest that the commit
log be something like:

    kernel/mingw64: new kernel

    Add mingw64 as a new kernel for x86_64.
    As a dependency, add the mingw64 C library.

    Add a sample to build a mingw64 toolchain.

    sob: you

If we can commonalise the two mingw variants, then I'd like to see
a patch series, with pathes like:
    [PATCH 1/2] kernel/mingw32: rename to mingw to preapare 64-bit variant
    [PATCH 2/2] kernel/mingw: add 64-bit variant

Use your imagination to complete the commit log. ;-)

[--SNIP--]
> diff -r 946d6d133a90 -r a34dced6ae17 config/kernel/mingw64.in
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/config/kernel/mingw64.in	Tue Oct 23 20:14:08 2012 +0200
> @@ -0,0 +1,33 @@

This file is *very* similar to the 32-bit variant, and would gain in being
folded together.

[--SNIP--]
> diff -r 946d6d133a90 -r a34dced6ae17 config/libc/mingw64.in
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/config/libc/mingw64.in	Tue Oct 23 20:14:08 2012 +0200
> @@ -0,0 +1,33 @@

Ditto, very similar.

> diff -r 946d6d133a90 -r a34dced6ae17 config/libc/mingw64.in.2
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/config/libc/mingw64.in.2	Tue Oct 23 20:14:08 2012 +0200
> @@ -0,0 +1,10 @@
> +# Part-2 of mingw C library options: development libraries
> +
> +config MINGW_DIRECTX
> +    bool
> +    prompt "Include DirectX development files"
> +
> +config MINGW_DDK
> +    bool
> +    prompt "Include DDK development files"
> +

As said above, these options should go in mingw64.in.

[--SNIP--]
> diff -r 946d6d133a90 -r a34dced6ae17 samples/x86_64-w64-mingw32/crosstool.config
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/samples/x86_64-w64-mingw32/crosstool.config	Tue Oct 23 20:14:08 2012 +0200
> @@ -0,0 +1,18 @@

Using a defconfig. Good! :-)

[--SNIP--]
> diff -r 946d6d133a90 -r a34dced6ae17 scripts/build/kernel/mingw64.sh
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/scripts/build/kernel/mingw64.sh	Tue Oct 23 20:14:08 2012 +0200
> @@ -0,0 +1,83 @@
> +# This file declares functions to install the kernel headers for mingw64
> +# Copyright 2012 Yann Diorcet
> +# Licensed under the GPL v2. See COPYING in the root of this package
> +
> +CT_DoKernelTupleValues() {
> +    CT_TARGET_KERNEL="mingw32"

mingw_32_, really?

[--SNIP--]
> +do_kernel_headers() {

You need to declare variables you use in this function as being local to
the function:
    local sdk_opts
    local -a mingw_opts

> +    CT_DoStep INFO "Installing kernel headers"
> +
> +    mkdir -p "${CT_HEADERS_DIR}"
> +#    cp -r ${CT_SRC_DIR}/mingw-w64-v${CT_W64API_VERSION}/include/*   \
> +#          ${CT_HEADERS_DIR}

Code commented means it is not used. Remove it.

> +    if [ -n "${CT_MINGW_DIRECTX}" ]; then

Do not expect that a non-empty string is equal to 'y'.
I prefer that we really check against 'y':
    if [ "${foo}" = "y" ]; then
        printf "foo is set to 'y'\n"
    fi

or against 'n':
    if [ -z "${foo}" ]; then
        printf "foo is not set (aka 'n')\n"
    fi

> +        sdk_opt="directx"
> +        if [ -n "${CT_MINGW_DDK}" ]; then
> +            sdk_opt="all"
> +        fi
> +    else
> +        if [ -n "${CT_MINGW_DDK}" ]; then
> +            sdk_opt="ddk"
> +        else
> +            sdk_opt="none"
> +        fi
> +    fi

Although your construct is corect, we usually use another one when there
are two (or three) bollean variables:

    case "${CT_MINGW_DIRECTX}:${CT_MINGW_DDK}" in
        y:y)    sdk_opt="all";;
        y:)     sdk_opt="directx";;
        :y)     sdk_opt="ddk";;
        :)      sdk_opt="none";;
    esac

Much more compact, as easy to read. ;-)

> +    CT_mkdir_pushd "${CT_BUILD_DIR}/build-header-build-${CT_BUILD}"
> +    gmp_opts+=( "host=${CT_TARGET}" )
> +    gmp_opts+=( "prefix=${CT_SYSROOT_DIR}" )
> +    gmp_opts+=( "sdk=${sdk_opt}" )

You're in the mingw kernel, not in gmp:
s/gmp_opts/mingw_opts/

> +    do_mingw64_headers_backend "${gmp_opts[@]}"
> +
> +    CT_Popd
> +
> +    CT_DoExecLog ALL cp -r ${CT_SYSROOT_DIR}/${CT_TARGET}/include/*   \
> +                     ${CT_HEADERS_DIR}

Always quotes variables, especially those that are paths.

> +    CT_EndStep
> +}
> +
> +# Build MINGW64 headers
> +#     Parameter     : description               : type      : default
> +#     host          : machine to run on         : tuple     : (none)
> +#     prefix        : prefix to install into    : dir       : (none)
> +#     cflags        : host cflags to use        : string    : (empty)

Last line shoule be something like:
   sdk    : sdk parts to build : string : (empty)

> +do_mingw64_headers_backend() {

Why do you need to use a backend here? In crosstool-NG, we use a backend
when the component needs to be built multiple times, not to abstract so
semantic.

There's no such need here.

> +    local host
> +    local prefix
> +    local cflags
> +    local arg
> +    local sdk
> +
> +    for arg in "$@"; do
> +        eval "${arg// /\\ }"
> +    done
> +
> +    CT_DoLog EXTRA "Configuring Headers"
> +
> +    CT_DoExecLog CFG                                \
> +    "${CT_SRC_DIR}/mingw-w64-v${CT_W64API_VERSION}/mingw-w64-headers/configure" \
> +	--build=${CT_BUILD}			    \
> +        --host=${host}                              \
> +        --prefix="${prefix}"                        \
> +        --enable-sdk="${sdk}"
> +
> +    CT_DoLog EXTRA "Compile Headers"
> +    CT_DoExecLog ALL make
> +
> +    CT_DoLog EXTRA "Installing Headers"
> +    CT_DoExecLog ALL make install
> +}
> diff -r 946d6d133a90 -r a34dced6ae17 scripts/build/libc/mingw64.sh
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/scripts/build/libc/mingw64.sh	Tue Oct 23 20:14:08 2012 +0200
> @@ -0,0 +1,52 @@
> +do_libc_get() { 

Nit-picking: trailing white space.

> +    CT_GetFile "mingw-w64-v${CT_W64API_VERSION}" \
> +        http://downloads.sourceforge.net/sourceforge/mingw-w64

Ah, I see that the mingw64 kernel and C library are from the same source
archive. Yes, you have to CT_GetFile nad CT_Extract it in both places.
Good.

> +}
> +
> +do_libc_extract() {
> +	CT_Extract "mingw-w64-v${CT_W64API_VERSION}"
> +}
> +
> +do_libc_check_config() {
> +    :
> +}
> +
> +do_libc_start_files() {
> +    CT_DoStep INFO "Installing C library headers"
> +
> +    # It seems mingw is strangely set up to look into /mingw instead of
> +    # /usr (notably when looking for the headers). This symlink is
> +    # here to workaround this, and seems to be here to last... :-/
> +    CT_DoExecLog ALL ln -sv "${CT_TARGET}" "${CT_SYSROOT_DIR}/mingw"

I do not like the way the sysroot is organised... :-/
I do not have enough bacgground in this 'kernel' to understand how
everything should fit together. :-(

Let's keep it that way for now...

> +    CT_EndStep
> +}
> +
> +do_libc() {
> +    CT_DoStep INFO "Building MinGW64 files"
> +
> +    CT_DoLog EXTRA "Configuring W64-CRT"
> +
> +    mkdir -p "${CT_BUILD_DIR}/build-w64crt"
> +    cd "${CT_BUILD_DIR}/build-w64crt"
> +
> +    CT_DoExecLog CFG                                                  \
> +    "${CT_SRC_DIR}/mingw-w64-v${CT_W64API_VERSION}/mingw-w64-crt/configure" \
> +        --prefix=${CT_SYSROOT_DIR}                                    \
> +        --host=${CT_TARGET}
> +
> +    CT_DoLog EXTRA "Building W64-CRT"
> +    CT_DoExecLog ALL make ${JOBSFLAGS}
> +
> +    CT_DoLog EXTRA "Installing W64-CRT"
> +    CT_DoExecLog ALL make install
> +
> +    CT_EndStep
> +}
> +
> +do_libc_finish() {
> +    CT_DoStep INFO "Installing MinGW64 Development libraries"
> +
> +    CT_EndStep
> +}

If this function deos nothing, no need to print anything at all (like in
the do_libc_check_config step).

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

--
For unsubscribe information see http://sourceware.org/lists.html#faq


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]