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] |
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] |