Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Alan Bateman]

On 10/11/2018 21:59, Magnus Ihse Bursie wrote:

I can't see that you've addressed any of the build system changes Erik and I 
requested? Or is this just a preliminary review, and you intend to go at least 
one more round before attempting to push? If so, this was not very clear to me.


I see Andy has a separate issue in JIRA tracking this:
  https://bugs.openjdk.java.net/browse/JDK-8212936

but I agree the bug management around this feature isn't clear to 
potential reviewers. My guess is he may be using JIRA issues to track 
feedback/changes between each iteration of the webrev so this is why 
affects and fixVersion are set to "internal". I think you need to make 
clear if the make file issues need to be addressed for the initial push 
- I suspect they do.


-Alan

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Andy Herrick]

yes of the nine build system issues brought up by Magnus and Erik 
(listed in JDK-8212936 ) , only one has been addressed so far.


This is at the top of the list of remaining issues we will be working ont:

The following additional issues are targeted to be address in the next few 
weeks:
JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
...

I may need some expert help  to implement some of these changes.

/Andy

On 11/11/2018 3:02 AM, Alan Bateman wrote:

On 10/11/2018 21:59, Magnus Ihse Bursie wrote:
I can't see that you've addressed any of the build system changes 
Erik and I requested? Or is this just a preliminary review, and you 
intend to go at least one more round before attempting to push? If 
so, this was not very clear to me.


I see Andy has a separate issue in JIRA tracking this:
  https://bugs.openjdk.java.net/browse/JDK-8212936

but I agree the bug management around this feature isn't clear to 
potential reviewers. My guess is he may be using JIRA issues to track 
feedback/changes between each iteration of the webrev so this is why 
affects and fixVersion are set to "internal". I think you need to make 
clear if the make file issues need to be addressed for the initial 
push - I suspect they do.


-Alan

Re: RFR: 8213622 - Windows VS2013 build failure - "'snprintf': identifier not found"

[Michal Vala]

On 11/10/18 12:07 AM, David Holmes wrote:

cc'ing JC Beyler as this was his code.

On 10/11/2018 4:35 AM, Kim Barrett wrote:

On Nov 9, 2018, at 11:43 AM, Michal Vala  wrote:

Hi,

please review following patch fixing build failure on Windows with VS2013 
toolchain.


Not sure we're still supporting VS2013 ... ??


Minimum accepted by configure is 2010. 2013 is 2nd in priorities after 2017. It 
has `VS_SUPPORTED_2013=false` though, but why not keep it buildable?






http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213622/webrev.00/


The failure is in a hotspot test.  It should be using os::snprintf.


Tests don't have access to os::snprintf. The test is just C++ code.

Cheers,
David




--
Michal Vala
OpenJDK QE
Red Hat Czech

Re: RFR: 8213591 : running bin/idea.sh in Cygwin: generated project cannot be imported - was : RE: bin/idea.sh and Cygwin

[Michal Vala]

Hi Thomas,

thanks! I've tested on Windows 2012, vs2013.

Anyone with latest Windows 10 to test this?

Also I'd like to ask someone to sponsor this, as I'm just an author.


On 11/9/18 7:09 PM, Thomas Stüfe wrote:

Hi Michal,

I tested this and it now works nicely for me (win7, vs2017, with
current jdk/jdk).

Change looks fine to me to.

Best Regards, Thomas

On Fri, Nov 9, 2018 at 7:23 PM Michal Vala  wrote:


I got valid idea project even with empty JT_HOME as placeholder was correctly
replaced by empty string. Sure that it's not acceptable.

Anyway, JT_HOME should be only variable that can be empty.

new webrev: http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213591/webrev.02/

On 11/9/18 5:42 PM, Thomas Stüfe wrote:

On Fri, Nov 9, 2018 at 5:35 PM Thomas Stüfe  wrote:


Hi Michal,

does not yet work for me. I get cygpath Usage output:

$ bash  ./bin/idea.sh
Usage: cygpath (-d|-m|-u|-w|-t TYPE) [-f FILE] [OPTION]... NAME...
 cygpath [-c HANDLE]
 cygpath [-ADHOPSW]
 cygpath [-F ID]

Convert Unix and Windows format paths, or output system path information
...

Cheers, Thomas


add_replacement "###JTREG_HOME###" "`cygpath -am $JT_HOME`"

seems to be the culprit.

JT_HOME is empty, and I never did set that before (I usually work on
Linux though).

I think the problem is that in this expression:

 if [ "x$CYGPATH" = "x" ]; then
 ..
 else
 ..
 fi

the non-windows path does not require the variables to be set. Whereas
calling "cygpath -am" without an argument is an error which leads to
the usage output.

..Thomas



On Fri, Nov 9, 2018 at 6:09 PM Michal Vala  wrote:


You're right, sorry. Updated webrev:
http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213591/webrev.01/

On 11/9/18 5:42 PM, Erik Joelsson wrote:

Hello Michal,

It looks like the "dirname" calls are omitted in the cygpath case, so BUILD_DIR
ends up pointing to the spec file instead of the directory the file is in.

/Erik


On 2018-11-09 05:58, Michal Vala wrote:

Hi,

I've looked into this. Please review the patch:
http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213591/webrev.00/

On 11/9/18 9:29 AM, Baesken, Matthias wrote:

Hello , I opened

8213591 :   running bin/idea.sh in Cygwin: generated project cannot be imported

https://bugs.openjdk.java.net/browse/JDK-8213591

for  the reported issue .

Best regards, Matthias


-Original Message-
From: Erik Joelsson 
Sent: Donnerstag, 8. November 2018 18:05
To: Baesken, Matthias ; Chris Hegarty
; 'build-dev@openjdk.java.net' ; maurizio.cimadam...@oracle.com
Subject: Re: bin/idea.sh and Cygwin

A patch fixing idea.sh so that it works on Windows would certainly be
welcome.

/Erik


On 2018-11-08 05:12, Baesken, Matthias wrote:

Hi  Chris ,  thanks for the info .

However I found out that replacing the   /cygdrive/C/ with C:/ in   the

top-level  xml/imlfiles in the  ".idea"  - folder

 makes   IntelliJ   happy,  I could then  open  the project successfully
from

IntelliJ  .


So I guess a  couple  of"cygpath  -aw"  -calls  at the right places  in
the

project generation   might   fix  the idea.sh   based project file
generation on
Cygwin   (without postprocessing).

Any comments on this ?

Or is there another  way  to get   .idea/-files  that open "out of the
box"   ?


Best regards, Matthias





-Original Message-
From: Chris Hegarty 
Sent: Donnerstag, 8. November 2018 12:52
To: Baesken, Matthias ; 'build-
d...@openjdk.java.net' ;
maurizio.cimadam...@oracle.com
Subject: Re: bin/idea.sh and Cygwin

Matthias,

On 08/11/18 11:45, Baesken, Matthias wrote:

Hello, I tried to use bin/idea.sh  with Cygwin to generate project files

for

IDEA  IntelliJ Community .

The  project file generation seems to work and outputs the   .idea -

folder

with lots of xml files in it .

However  ,  when opening the project from IDEA,  it fails  with  a

message :


VCS root  configuration  problems  -

The directory  \cygdrive\C\hg\open\jdk\jdk6 is registered as a hg4idea

root

but no hg4idea  repositories were found  there .

C.\hg\open\jdk\jdk6


Could it be that  the Cygwin-paths  in the generated xml-files confuse

the

IDEA intelliJ IDE ?

Certainly looks like it.


Has anybody ever used it successfully  with Cygwin/ Windows  ?
( or with some other UNIX shell/toolset for Windows) ?

I have not tried. I use it successfully on macOS and Linux.

-Chris.








--
Michal Vala
OpenJDK QE
Red Hat Czech


--
Michal Vala
OpenJDK QE
Red Hat Czech


--
Michal Vala
OpenJDK QE
Red Hat Czech

Re: RFR: 8213622 - Windows VS2013 build failure - "'snprintf': identifier not found"

[Kim Barrett]

> On Nov 9, 2018, at 6:07 PM, David Holmes  wrote:
> 
> cc'ing JC Beyler as this was his code.
> 
> On 10/11/2018 4:35 AM, Kim Barrett wrote:
>>> On Nov 9, 2018, at 11:43 AM, Michal Vala  wrote:
>>> 
>>> Hi,
>>> 
>>> please review following patch fixing build failure on Windows with VS2013 
>>> toolchain.
> 
> Not sure we're still supporting VS2013 ... ??

For now, we still claim to support VS2013; it's listed in the
supported versions in make/autoconf/toolchains_windows.m4.  That will
change with JDK-8208089 (if not sooner).

>>> http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213622/webrev.00/
>> The failure is in a hotspot test.  It should be using os::snprintf.
> 
> Tests don't have access to os::snprintf. The test is just C++ code.

Drat!  Yeah, we don't seem to have access to VM code from these test
support codes.  That's too bad.

However, the proposed change doesn't work as intended.

There are two calls to snprintf in ExceptionCheckingJniEnv.cpp, which
would be affected by the proposed change. The first is calling
snprintf with an empty buffer to determine the size of the needed
buffer. _snprintf returns -1 on buffer overflow, rather than the
number of characters that would have been printed if the buffer size
was sufficient to hold the output. So replacing that snprintf call
with _snprintf will return a very different result from what is
expected. I don't see a simple way to solve this while retaining the
source calls to snprintf.

That first snprintf call could be rewritten using several calls to
strlen to calculate the needed size in some different manner. The
later call could also be rewritten to use several calls to strcpy
rather than snprintf.

I think there currently aren't any other calls that would be affected,
even though there are other #includes of the file being changed.  But
that's not trivial to check, and could change, possibly leading to
hard to fathom bugs.

There are 8 other calls to snprintf in test/hotspot/jtreg.  Most of
them are linux-only or solaris-only, so don't trip over the lack of
snprintf on windows.

However, one if them, in jvmti_tools.cpp, I think is applicable to
Windows.  It builds because it uses the renaming macro approach, with
the macro being in jvmti_tools.h.  Yuck (IMO).

Re: RFR: 8213622 - Windows VS2013 build failure - "'snprintf': identifier not found"

[David Holmes]

Hi Michal,

On 12/11/2018 2:18 PM, Michal Vala wrote:



On 11/10/18 12:07 AM, David Holmes wrote:

cc'ing JC Beyler as this was his code.

On 10/11/2018 4:35 AM, Kim Barrett wrote:

On Nov 9, 2018, at 11:43 AM, Michal Vala  wrote:

Hi,

please review following patch fixing build failure on Windows with 
VS2013 toolchain.


Not sure we're still supporting VS2013 ... ??


Minimum accepted by configure is 2010. 2013 is 2nd in priorities after 
2017. It has `VS_SUPPORTED_2013=false` though, but why not keep it 
buildable?


That depends on how painful it is to achieve that. As Kim has explained 
the suggested fix may allow building but it isn't correct.


And we may not be able to keep this ability to build with VS2013 going 
forward.


Thanks,
David






http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213622/webrev.00/


The failure is in a hotspot test.  It should be using os::snprintf.


Tests don't have access to os::snprintf. The test is just C++ code.

Cheers,
David

Re: RFR: 8213591 : running bin/idea.sh in Cygwin: generated project cannot be imported - was : RE: bin/idea.sh and Cygwin

[Thomas Stüfe]

Hi Michal,

I can sponsor for you.

@Matthias: yould you test on your Box too if this patch works?

Best Regards, Thomas

On Mon, Nov 12, 2018 at 5:24 AM Michal Vala  wrote:
>
> Hi Thomas,
>
> thanks! I've tested on Windows 2012, vs2013.
>
> Anyone with latest Windows 10 to test this?
>
> Also I'd like to ask someone to sponsor this, as I'm just an author.
>
>
> On 11/9/18 7:09 PM, Thomas Stüfe wrote:
> > Hi Michal,
> >
> > I tested this and it now works nicely for me (win7, vs2017, with
> > current jdk/jdk).
> >
> > Change looks fine to me to.
> >
> > Best Regards, Thomas
> >
> > On Fri, Nov 9, 2018 at 7:23 PM Michal Vala  wrote:
> >>
> >> I got valid idea project even with empty JT_HOME as placeholder was 
> >> correctly
> >> replaced by empty string. Sure that it's not acceptable.
> >>
> >> Anyway, JT_HOME should be only variable that can be empty.
> >>
> >> new webrev: 
> >> http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213591/webrev.02/
> >>
> >> On 11/9/18 5:42 PM, Thomas Stüfe wrote:
> >>> On Fri, Nov 9, 2018 at 5:35 PM Thomas Stüfe  
> >>> wrote:
> 
>  Hi Michal,
> 
>  does not yet work for me. I get cygpath Usage output:
> 
>  $ bash  ./bin/idea.sh
>  Usage: cygpath (-d|-m|-u|-w|-t TYPE) [-f FILE] [OPTION]... NAME...
>   cygpath [-c HANDLE]
>   cygpath [-ADHOPSW]
>   cygpath [-F ID]
> 
>  Convert Unix and Windows format paths, or output system path information
>  ...
> 
>  Cheers, Thomas
> >>>
> >>> add_replacement "###JTREG_HOME###" "`cygpath -am $JT_HOME`"
> >>>
> >>> seems to be the culprit.
> >>>
> >>> JT_HOME is empty, and I never did set that before (I usually work on
> >>> Linux though).
> >>>
> >>> I think the problem is that in this expression:
> >>>
> >>>  if [ "x$CYGPATH" = "x" ]; then
> >>>  ..
> >>>  else
> >>>  ..
> >>>  fi
> >>>
> >>> the non-windows path does not require the variables to be set. Whereas
> >>> calling "cygpath -am" without an argument is an error which leads to
> >>> the usage output.
> >>>
> >>> ..Thomas
> >>>
> 
>  On Fri, Nov 9, 2018 at 6:09 PM Michal Vala  wrote:
> >
> > You're right, sorry. Updated webrev:
> > http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213591/webrev.01/
> >
> > On 11/9/18 5:42 PM, Erik Joelsson wrote:
> >> Hello Michal,
> >>
> >> It looks like the "dirname" calls are omitted in the cygpath case, so 
> >> BUILD_DIR
> >> ends up pointing to the spec file instead of the directory the file is 
> >> in.
> >>
> >> /Erik
> >>
> >>
> >> On 2018-11-09 05:58, Michal Vala wrote:
> >>> Hi,
> >>>
> >>> I've looked into this. Please review the patch:
> >>> http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213591/webrev.00/
> >>>
> >>> On 11/9/18 9:29 AM, Baesken, Matthias wrote:
>  Hello , I opened
> 
>  8213591 :   running bin/idea.sh in Cygwin: generated project cannot 
>  be imported
> 
>  https://bugs.openjdk.java.net/browse/JDK-8213591
> 
>  for  the reported issue .
> 
>  Best regards, Matthias
> 
> > -Original Message-
> > From: Erik Joelsson 
> > Sent: Donnerstag, 8. November 2018 18:05
> > To: Baesken, Matthias ; Chris Hegarty
> > ; 'build-dev@openjdk.java.net'  > d...@openjdk.java.net>; maurizio.cimadam...@oracle.com
> > Subject: Re: bin/idea.sh and Cygwin
> >
> > A patch fixing idea.sh so that it works on Windows would certainly 
> > be
> > welcome.
> >
> > /Erik
> >
> >
> > On 2018-11-08 05:12, Baesken, Matthias wrote:
> >> Hi  Chris ,  thanks for the info .
> >>
> >> However I found out that replacing the   /cygdrive/C/ with C:/ in  
> >>  the
> > top-level  xml/imlfiles in the  ".idea"  - folder
> >>  makes   IntelliJ   happy,  I could then  open  the project 
> >> successfully
> >> from
> > IntelliJ  .
> >>
> >> So I guess a  couple  of"cygpath  -aw"  -calls  at the right 
> >> places  in
> >> the
> > project generation   might   fix  the idea.sh   based project file
> > generation on
> > Cygwin   (without postprocessing).
> >> Any comments on this ?
> >>
> >> Or is there another  way  to get   .idea/-files  that open "out of 
> >> the
> >> box"   ?
> >>
> >>
> >> Best regards, Matthias
> >>
> >>
> >>
> >>
> >>> -Original Message-
> >>> From: Chris Hegarty 
> >>> Sent: Donnerstag, 8. November 2018 12:52
> >>> To: Baesken, Matthias ; 'build-
> >>> d...@openjdk.java.net' ;
> >>> maurizio.cimadam...@oracle.com
> >>> Subject: Re: b

RFR: 8213569: Bump minimum boot jdk to JDK 11

[Mikael Vidstedt]

Please review this change which bumps the minimum boot jdk to JDK 11 (see [1] 
for Boot-JDK policy).

bug: https://bugs.openjdk.java.net/browse/JDK-8213569 

webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8213569/webrev.00/open/webrev/ 


* Background

JDK 11 is now GA. The minimum boot JDK version for mainline/JDK 12 should be 
bumped.


In addition to bumping the minimum boot JDK version this change also updates 
the boot JDK used by the Oracle build system.

* Testing

tier1-3 (found one unrelated/known failure).

Cheers,
Mikael

[1] http://mail.openjdk.java.net/pipermail/jdk-dev/2018-April/001075.html