Hi Phil, sorry I didn't wanted to overrule you but after I didn't heard back from you for almost a week (an out of office mail could have been helpful) and after I got 5 other reviews I decided to finally fix the build error on AIX.
On Mon, Nov 26, 2018 at 9:35 PM Phil Race <philip.r...@oracle.com> wrote: > > Well .. I see this was pushed whilst I was on vacation. > I would not have voted for the fix in its current form. The > "HAVE_XRANDR" would > have been less bad. If (say) HPUX has the same issue you > haven't really helped them with an AIX specific change in the source file. > It would have been better to confine all of that decision making to the > make files. > > Can we re-do this ? And since it'll be less urgent the follow up can be > pushed to jdk/client. > Sure, I have opened "8214343: Handle the absence of Xrandr more generically" [1] for it. I'll send out a RFR soon. [1] https://bugs.openjdk.java.net/browse/JDK-8214343 > But I really don't get why AIX doesn't have XRANDR. > I understood that it might not have GDK - which is why - you may remember - > we checked into that when looking at options for the Robot imported sources > and deferred that, but XRANDR ??? I think this is an AIX "bug" which should > be reported to IBM. > I have reported it, but in the end AIX, HP-UX, Solaris, etc. are all zombie operating systems :( > -phil. > > > On 11/20/18 10:13 AM, Volker Simonis wrote: > > Thanks everybody for the reviews. > > > > If nobody raises a "Veto" (Phil?) I plan to push this fix tomorrow in > > its current form. > > > > I've also run it through the submit repo and got an error on Windows > > for the test "runtime/modules/JVMDefineModule.java" which seems > > completely unrelated to my change which only touches the X11 > > implementation on Unix. Can somebody please confirm that? > > > > [Mach5] mach5-one-simonis-JDK-8213944-20181120-1629-11082: FAILED, > > Failed tests: 1 > > > > runtime/modules/JVMDefineModule.java tier1 windows-x64-debug othervm > > driver ExitCode: -1073741819 > > > > Mach5 Tasks Results Summary > > > > UNABLE_TO_RUN: 0 > > FAILED: 0 > > EXECUTED_WITH_FAILURE: 1 > > KILLED: 0 > > PASSED: 75 > > NA: 0 > > > > Thank you and best regards, > > Volker > > > > On Tue, Nov 20, 2018 at 12:05 PM Magnus Ihse Bursie > > <magnus.ihse.bur...@oracle.com> wrote: > >> > >> > >> On 2018-11-19 18:56, Volker Simonis wrote: > >>> Hi Phil, > >>> > >>> I'd like to kindly ask you to suggest how we can proceed with this issue. > >>> > >>> As I wrote before, Xrandr is not officially supported on AIX and there > >>> are no official packages available for it. There are some OpenSource > >>> sites for AIX which provide Xrandr, but they are all not compatible > >>> with the default native libraries (e.g. the open source Xrandr package > >>> depends on another open source package of Xrender.so.1 but the system > >>> only provides Xrender.so.0 natively). We can't compile the whole JDK > >>> (i.e. libawt_xawt.so) against some open source package of Xrender.so.1 > >>> because that simply won't be available on the majority of systems. > >>> > >>> Remember that forcing people to install these open-source packages > >>> even as a build dependency is like expecting Linux users to install > >>> some non-official packages just to be able to build. Especially in > >>> corporate environments that's not easy. Moreover the benefit would be > >>> really minimal, because the Xrandr functionality won't be available at > >>> runtime anyway. > >>> > >>> So to cut a long story short, I see two options: > >>> > >>> 1. Go with my current patch (ugly but efficient) > >>> > >>> 2. Check-in in an AIX specific version of XRander.h/randr.h under > >>> src/java.desktop/aix (OK for me, but that would actually negate the > >>> initial purpose of "8210863: Remove Xrandr include files") > >>> > >>> Do you have a better proposal? > >> I think the change look good, and I vote for strategy 1. As Thomas > >> suggested, if the AIX ifdefs look bad we can create a new define, but > >> I'm not sure that's really helpful - after all, it's just on AIX we > >> currently have no r&r. Having a define would mostly be needed if it was > >> multiple OSes, or similar more complex situations, that would have/not > >> have the r&r extension. > >> > >> Yet another solution, to get rid of the ifdefs, is to move the relevant > >> Xranrd dependent functions into a new, separate file, like > >> awt_GraphicsEnv_randr.c, and then in the build exclude it on AIX (or, > >> perhaps if it's worth the trouble, on all platforms where configure did > >> not find Xrandr). > >> > >> /Magnus > >> > >>> Thank you and best regards, > >>> Volker > >>> > >>> On Fri, Nov 16, 2018 at 11:22 AM Volker Simonis > >>> <volker.simo...@gmail.com> wrote: > >>>> On Thu, Nov 15, 2018 at 6:01 PM Philip Race <philip.r...@oracle.com> > >>>> wrote: > >>>>> PS I am not sure why xrandr headers would not be available for AIX. > >>>>> They are a standard part of the xdistribution. > >>>>> > >>>> I'm not an X11 guru, but as far as I understand, xrandr is an > >>>> extension and as such it doesn't have to be supported by every > >>>> implementation. To the best of my knowledge (I've just started another > >>>> poll among some experts) AIX doesn't support Xrandr and does not have > >>>> the corresponding headers. > >>>> > >>>>> If true, think what you are going to have to do is add a > >>>>> --with-xrandr-include option > >>>>> and provide it that way. > >>>>> > >>>> What if there are no standard Xrandr headers on a platform? Do you > >>>> really want to force users to get them from some dubious sources just > >>>> for building the OpenJDK? Sorry, but I don't think that's a good > >>>> solution. Than I'd rather prefer the ugly ifdefs (or I check the > >>>> headers back in again in an AIX-specific directory :) > >>>> > >>>> Thank you and best regards, > >>>> Volker > >>>> > >>>>> -phil. > >>>>> > >>>>> On 11/15/18, 8:55 AM, Philip Race wrote: > >>>>>> Hmm. I don't like the ifdefs. > >>>>>> > >>>>>> Xrandr is a requirement for the build. If its not there at runtime > >>>>>> that's OK. > >>>>>> > >>>>>> -phil. > >>>>>> > >>>>>> On 11/15/18, 8:06 AM, Volker Simonis wrote: > >>>>>>> Hi, > >>>>>>> > >>>>>>> can I please have a review for the following small change: > >>>>>>> > >>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8213944/ > >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8213944 > >>>>>>> > >>>>>>> Change JDK-8210863 removed the Xrandr.h/randr.h headers from the > >>>>>>> OpenJDK sources but forgot to add a configure check for the Xrandr > >>>>>>> extension which is now a build dependency. > >>>>>>> > >>>>>>> The change also broke the AIX build. AIX never supported Xrandr, but > >>>>>>> that was only detected at runtime, when the JDK was unable to > >>>>>>> dynamically load libXrand.so. Now, without Xrandr.h/randr.h in the > >>>>>>> source tree any more, we have to conditionally compile some parts of > >>>>>>> src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c such > >>>>>>> that it doesn't require the definitions and declarations from > >>>>>>> Xrandr.h/randr.h any more. > >>>>>>> > >>>>>>> Thank you and best regards, > >>>>>>> Volker >