Sorry, I missed your question. I can't speak for the aix-port repo, but I'm satisfied from the build point of view.
/Magnus 10 sep 2013 kl. 14:22 skrev Volker Simonis <volker.simo...@gmail.com>: > > > > On Mon, Sep 9, 2013 at 4:32 PM, Volker Simonis <volker.simo...@gmail.com> > wrote: >> Hi Magnus, >> >> thanks again for the review. Please see my comments inline: >> >> >> On Mon, Sep 9, 2013 at 12:23 PM, Magnus Ihse Bursie >> <magnus.ihse.bur...@oracle.com> wrote: >>> Hi Volker, >>> >>> Some more comments inlined. >>> >>>> OK, common/autoconf/build-aux/autoconf-config.guess was too old and didn't >>>> knew about AIX 7 so it returned the default AIX fallback which is >>>> 'rs6000-ibm-aix'. I have now fixed 'autoconf-config.guess' to know about >>>> AIX 7 (a one character change which is already in autoconf-2.69 so we >>>> won't have problems if you should ever update >>>> autoconf-config.guess). >>> >>> Hmm... While more elegant, the idea was that autoconf-config.guess should >>> be a strict copy of the config.guess file from the autoconf package. The >>> whole idea of wrapping it in a custom config.guess was to avoid "forking" >>> the autoconf config.guess, with small changes (like this) that would be >>> hard to track and might get lost if we update to a newer version from the >>> upstream autoconf file. >>> >>> That being said, since we settled on autoconf 2.69, we really should update >>> the file to config.guess from autoconf-2.69. I've started a test run on our >>> internal test system with the config.guess from 2.69 to see if it breaks >>> any existing platforms. If not, I suggest we do a fast integration of the >>> new config.guess. >>> >>> Can you hold on with your changes until a new config.guess comes in? Even >>> if you're just making a small change that will be reverted soon after, I >>> think it would set a unfortunate precedent. >> >> That's OK for me. Actually my change still works without the changes in >> "config.guess" on AIX 5.3. And it will automatically start working once we >> get the new "config.guess". >> >>>> >>>>> Ahrgh, all these proud compilers with their own ways of expressing the >>>>> same functionality. :( I assume that you are using the COMP_MODE_OPTION >>>>> in the jdk projct? I couldn't find any references to it in the Hotspot >>>>> build changes, and otherwise there seems to be no reason to export it in >>>>> the spec.gmk file. >>>> >>>> Yes, exactly. It is used in 'jdk/makefiles/GensrcX11Wrappers.gmk' >>>> (MEMORY_MODEL_FLAG="$(COMP_MODE_OPTION)$*"). >>> Once again the X11 wrappers. We should really make an effort and get rid of >>> them. :-/ >> >> I don't mind:) >>>> I fully agree with the criticism on the name:) After we already have >>>> 'COMPILER_SUPPORTS_TARGET_BITS_FLAG' I've simply renamed it to >>>> 'COMPILER_TARGET_BITS_FLAG'. I think that's much more appropriate and if >>>> you don't like it we should ask the one who invented >>>> 'COMPILER_SUPPORTS_TARGET_BITS_FLAG' :) And I had to set >>>> 'COMPILER_TARGET_BITS_FLAG' a little earlier such that is availabel in >>>> PLATFORM_SET_COMPILER_TARGET_BITS_FLAGS. >>> Sounds good! Nice aligning with the existing macro. >>> >>> However, you forgot to change the name in spec.gmk.in. (At least in the >>> webrev you published.) >> >> Good catch! I didn't realize it because the build in the stage repositories >> currently doesn’t reach the X11 wrappers. >> I've changed it and moved it near the definition of >> 'COMPILER_SUPPORTS_TARGET_BITS_FLAG' >> >>> >>>> 2. After you pointed out that setting '-q64' as extra flags on the >>>> configure command line is not the way it is supposed to work I recalled >>>> that we also have this problem on older Linux/PPC64 boxes (e.g. SLES 10) >>>> where the default compiler produces 32-bit objects by default. To fix this >>>> problem as well, I've inserted a call to >>>> PLATFORM_SET_COMPILER_TARGET_BITS_FLAGS followed by a second call to >>>> AC_CHECK_SIZEOF([int *], [1111]) in the case where we would otherwise have >>>> bailed out because the "TESTED_TARGET_CPU_BITS" differs from the actual >>>> "OPENJDK_TARGET_CPU_BITS". I think this change should not >>>> affect any existing platforms because it will only be triggered where we >>>> woould have bailed out with an error anyway. >>>> >>>> Also, the workaround for autoconf bug >>>> http://lists.gnu.org/archive/html/autoconf/2010-07/msg00004.html >>>> in AC_CHECK_SIZEOF isn't needed any more now that we require at least >>>> autoconf-2.69 because the problem was fixed in 2.67. And if I looked at it >>>> more carefully I must say that I don't understand the workaround at all. >>>> In my opinion, the test "x$SIZEOF_INT_P" != "x$ac_cv_sizeof_int_p" will >>>> always fail, because the AC_CHECK_SIZEOF macro only writes a define for >>>> SIZEOF_INT_P into "confdefs.h" (as can be seen in generated-configure.h) >>>> but it never defines it in the shell. And defining SIZEOF_INT_P in the >>>> configure shell script wouldn't help if the define written by the >>>> AC_CHECK_SIZEOF macro was wrong (as described in the bug). So Ithink the >>>> best is to remove the workaround and use >>>> "ac_cv_sizeof_int_p" in the places where we used AC_CHECK_SIZEOF before. >>> >>> Good to get rid of the old workaround. I agree, it looks kind of weird. I >>> think I might have been behind some of the weirdness; I think I interpreted >>> the autoconf documentation as if it should assign a variable SIZEOF_INT_P >>> in the configure script, and that the $ac_* variables were internal >>> variables that should not be directly accessed. In the current >>> documentation, at least, the $ac_cv_sizeof* macro is officially mentioned >>> so it should be safe to use. >>> >>> However, your second relies on some internal autoconf magic, by >>> unsetting variables and defines. We've tried to avoid that, but at times >>> there were no choice. Since we're about to fail anyway, and the code is >>> more in place for future, strange platforms, it's probably no harm. >> >> Exactly. >> >> Is it OK if I push it now to >> http://hg.openjdk.java.net/ppc-aix-port/ppc-aix-port/stage or is there >> anything you want to test first? > > Vladimir will push the changes because he also needs to re-generate the > closed config files (thanks Vladimir). > > Here's the final webrev: > > http://cr.openjdk.java.net/~simonis/webrevs/8024265.v3 > > @Vladimir: I used autoconf 2.69 to create the webrev, so if you recreate the > config files the diff to 'common/autoconf/generated-configure.sh' should be > only the time-stamp field. > > Regards, > Volker > >> >>> /Magnus >
