date:20170519

Hi,

On 05/19/2017 12:55 PM, Erik Joelsson wrote:

Sure we can pause this. Robbin has a local patch to play with for now.

Yes, no rush for me.

If you want another name, then we should also rename exploded-image. As I understand it, the name exploded is well established. It refers to the java class files/resources
not being in jars/jimage format, but exploded as separate files in a directory structure.

I would prefer to keep the name exploded for these reasons.

/Robbin

/Erik

On 2017-05-19 11:45, Magnus Ihse Bursie wrote:
I like the idea, the changes in itself look good, but I really *really* do not like the name "exploded". It was not good before as in "exploded-image", but this is even
worse. :-(

Can we pause this one for just a while and try really hard to come up with a better name? If we fail to do that in a couple of days, I'll admit defeat and accept the
patch as it is. OK?

What about "local" image? "simple"? "quick"? "bare"?

/Magnus

On 2017-05-19 11:26, Erik Joelsson wrote:
In most cases, when running regression tests, you want to run them on the jdk image because that's the correct image and some tests will not work on other variants.
However, many tests can be run successfully on the exploded image and since building the full jdk image takes quite a bit of extra time, especially when running a
slowdebug build, enabling this as an option can significantly improve developer efficiency.

I therefor propose the new target "exploded-run-test". It will work just like
run-test but it depends on and runs on the exploded image.

Bug: https://bugs.openjdk.java.net/browse/JDK-8180651

Webrev: http://cr.openjdk.java.net/~erikj/8180651/webrev.01/

/Erik

Re: RFR: JDK-8175824 Adapt javadoc generation to different requirements for JDK and JavaSE

2017-05-19 Thread Mandy Chung

Magnus,

Iris and Kevin provides the description for each group.  The overview page
can look like this:

This document is the Java™ Platform, Standard Edition Development Kit (JDK™) 9  
API Specification.

Java SE
The Java Platform, Standard Edition (“Java SE”) APIs define 
the core Java platform for general-purpose computing. These APIs
are in modules with names starting with the string “java.”.

JDK
The Java Development Kit (“JDK”) APIs define an implementation
of the Java SE Platform which may include platform-specific details. 
These APIs are in modules with names starting with the string “jdk.”.

JavaFX
The JavaFX APIs define a set of user interface (UI) controls,
graphics, media, and web packages for developing rich client 
applications. These APIs are in modules with names starting with 
the string "javafx.”.

Java SE, JDK, JavaFX links to the corresponding table.

Mandy

> On May 19, 2017, at 2:28 AM, Magnus Ihse Bursie 
>  wrote:
> 
> javadoc allows a file to be specified to provide content for the top level 
> "overview" page.
> 
> As we move towards a unified docs bundle, we need to be able to vary the 
> content of that file depending on the content of the bundle. This does not 
> mean providing or including lists of modules, but should reflect whether the 
> bundle contains Java SE modules, JDK modules and JavaFX modules.
> 
> This patch also includes support for (and relies on) the new usage of the 
> -group option from JDK-8180336.
> 
> Note that when JDK-8180480 (Use "requires transitive" relationship when 
> determining modules for javadoc) is pushed, the call to 
> FindTransitiveDepsForModules for JavaSE_MODULES should be updated to the new 
> FindTransitiveIndirectExportsForModules (or whatever it'll end up being 
> called). I'll fix that in whichever of the two bugs I push last.
> 
> This patch also contains some accumulated cleanup in Docs.gmk after all the 
> piecemeal patches this file has recieved for the last few weeks.
> 
> Note: While I'm willing to make minor changes to the actual contents of the 
> generated overview.html, I'd prefer if any larger (or potentially 
> controversial) issues are handled as separate follow-up bugs. With this 
> framework in place, it's easy to modify whatever gets written to the file.
> 
> An example how this looks like for the JDK and Java SE documentation 
> collections (stripped down to the summary page of the javadoc output, and the 
> JDK top-level index.html) is here:
> http://cr.openjdk.java.net/~ihse/demo-generated-overview/
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8175824
> WebRev: 
> http://cr.openjdk.java.net/~ihse/JDK-8175824-javadoc-dynamic-overview-and-groups/webrev.01
>

Re: RFR: JDK-8180480 Use "requires transitive" relationship when determining modules for javadoc

2017-05-19 Thread Mandy Chung


> On May 19, 2017, at 1:08 AM, Magnus Ihse Bursie 
>  wrote:
> 
> 
> "Indirect exports" was a good term. I dropped the "Deps" part of the name and 
> it all became much clearer.
> 
> Here's an updated webrev:
> http://cr.openjdk.java.net/~ihse/JDK-8180480-docs-should-use-requires-transitive/webrev.02
> 

+  $1_INDIRECT_EXPORTS := $$(call FindTransitiveIndirectExportsForModules, 
$$($1_MODULES))

Nit: “exported” term is for API but not for dependency.  

s/$1_INDIRECT_EXPORTS/$1_TRANSITIVE_MODULES
s/FindTransitiveIndirectExportsForModules/FindTransitiveIndirectDepsForModules

Otherwise, looks good.  No need for a new webrev:

Mandy

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems


Hi David

On 05/19/2017 01:36 PM, David Holmes wrote:


There are three different forms of the calculation. The two relative time versions use a different time function and so a different time structure (timeval vs timespec) 
and a different calculation.


Yes that's why I included unit in my example signature.



It's more complicated than that because we may have build time 
SUPPORTS_CLOCK_MONOTONIC but we still need the runtime check as well.

to_abstime is the old linux unpackTime with the addition of the build time 
conditionals.


I'm not changing that, I'm not sure how to explain better,
so here is the patch (only compiled and silly naming convention):
http://cr.openjdk.java.net/~rehn/8174231/webrev/

This makes the calculation independent of source/unit.

Thanks!

/Robbin





David


David
-


I do not see a problem with this, only better readability?

/Robbin



Thanks,
David
-


  struct timespec now;
  int status = _clock_gettime(CLOCK_MONOTONIC, );
  assert_status(status == 0, status, "clock_gettime");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_nsec, 
NANOUNITS);
   } else {
#else
   {
#endif
  struct timeval now;
  int status = gettimeofday(, NULL);
  assert(status == 0, "gettimeofday");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_usec, 
MICROUNITS);
   }
#endif

Thanks for fixing this!

/Robbin

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems


Correction ...

On 19/05/2017 8:53 PM, David Holmes wrote:

On 19/05/2017 7:25 PM, Robbin Ehn wrote:

On 05/19/2017 11:07 AM, David Holmes wrote:


They have to be as there are three cases:

1. Relative wait using CLOCK_MONOTONIC
2. Relative wait using gettimeofday()
3. Absolute wait using gettimeofday()


Please consider something like:

#ifdef SUPPORTS_CLOCK_MONOTONIC
   if (_use_clock_monotonic_condattr  && !isAbsolute) { // Why 
aren't we using this when not isAbsolute is set?
// I suggest removing that check 
from this if and use monotonic for that also.


Absolute waits have to be based on wall-clock time and follow any 
adjustments made to wall clock time. In contrast relative waits 
should never be affected by wall-clock time adjustments hence the use 
of CLOCK_MONOTONIC when available.


In Java the relative timed-waits are:
- Thread.sleep(ms)
- Object.wait(ms)/wait(ms,ns)
- LockSupport.parkNanos(ns) (and all the j.u.c blocking methods built 
on top of it)


While the only absolute timed-wait we have is the 
LockSupport.parkUntil method(s).


Hope that clarifies things.


Yes thanks!

But you can still re-factoring to something similar to what I 
suggested and two of the calculation should be the same just ns vs us, 
correct?


There are three different forms of the calculation. The two relative 
time versions use a different time function and so a different time 
structure (timeval vs timespec) and a different calculation.


Leaving the if statement with the "!isAbsolute" check, in my head 
calc_time is something like:


void calc_time(...) {
   if (isAbsolute) {
 calc_abs_time(...);
   } else {

#ifdef SUPPORTS_CLOCK_MONOTONIC

 calc_rel_time_from_clock_monotonic(...);

#else
 >  calc_rel_time_from_gettimeofday(...);
#endif

   }
}


It's more complicated than that because we may have build time 
SUPPORTS_CLOCK_MONOTONIC but we still need the runtime check as well.


to_abstime is the old linux unpackTime with the addition of the build 
time conditionals.


David


David
-


I do not see a problem with this, only better readability?

/Robbin



Thanks,
David
-


  struct timespec now;
  int status = _clock_gettime(CLOCK_MONOTONIC, );
  assert_status(status == 0, status, "clock_gettime");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, 
now.tv_nsec, NANOUNITS);

   } else {
#else
   {
#endif
  struct timeval now;
  int status = gettimeofday(, NULL);
  assert(status == 0, "gettimeofday");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, 
now.tv_usec, MICROUNITS);

   }
#endif

Thanks for fixing this!

/Robbin

Re: RFR: JDK-8175824 Adapt javadoc generation to different requirements for JDK and JavaSE

Looks good.

Minor nit. There are variables named JavaSE_MODULES as well as
JAVASE_MODULES which can be a bit confusing. Perhaps the latter can be
inlined at this point? Or the first should perhaps be called
JavaSE_GROUP_MODULES?

/Erik

On 2017-05-19 11:28, Magnus Ihse Bursie wrote:
javadoc allows a file to be specified to provide content for the top
level "overview" page.

As we move towards a unified docs bundle, we need to be able to vary
the content of that file depending on the content of the bundle. This
does not mean providing or including lists of modules, but should
reflect whether the bundle contains Java SE modules, JDK modules and
JavaFX modules.

This patch also includes support for (and relies on) the new usage of
the -group option from JDK-8180336.

Note that when JDK-8180480 (Use "requires transitive" relationship
when determining modules for javadoc) is pushed, the call to
FindTransitiveDepsForModules for JavaSE_MODULES should be updated to
the new FindTransitiveIndirectExportsForModules (or whatever it'll end
up being called). I'll fix that in whichever of the two bugs I push last.

This patch also contains some accumulated cleanup in Docs.gmk after
all the piecemeal patches this file has recieved for the last few weeks.

Note: While I'm willing to make minor changes to the actual contents
of the generated overview.html, I'd prefer if any larger (or
potentially controversial) issues are handled as separate follow-up
bugs. With this framework in place, it's easy to modify whatever gets
written to the file.

An example how this looks like for the JDK and Java SE documentation
collections (stripped down to the summary page of the javadoc output,
and the JDK top-level index.html) is here:

http://cr.openjdk.java.net/~ihse/demo-generated-overview/

Bug: https://bugs.openjdk.java.net/browse/JDK-8175824
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8175824-javadoc-dynamic-overview-and-groups/webrev.01

Re: RFR: JDK-8180651: Make target to run tests on exploded image


Sure we can pause this. Robbin has a local patch to play with for now.

If you want another name, then we should also rename exploded-image. As 
I understand it, the name exploded is well established. It refers to the 
java class files/resources not being in jars/jimage format, but exploded 
as separate files in a directory structure.


/Erik


On 2017-05-19 11:45, Magnus Ihse Bursie wrote:
I like the idea, the changes in itself look good, but I really 
*really* do not like the name "exploded". It was not good before as in 
"exploded-image", but this is even worse. :-(


Can we pause this one for just a while and try really hard to come up 
with a better name? If we fail to do that in a couple of days, I'll 
admit defeat and accept the patch as it is. OK?


What about "local" image? "simple"? "quick"? "bare"?

/Magnus

On 2017-05-19 11:26, Erik Joelsson wrote:
In most cases, when running regression tests, you want to run them on 
the jdk image because that's the correct image and some tests will 
not work on other variants. However, many tests can be run 
successfully on the exploded image and since building the full jdk 
image takes quite a bit of extra time, especially when running a 
slowdebug build, enabling this as an option can significantly improve 
developer efficiency.


I therefor propose the new target "exploded-run-test". It will work 
just like run-test but it depends on and runs on the exploded image.


Bug: https://bugs.openjdk.java.net/browse/JDK-8180651

Webrev: http://cr.openjdk.java.net/~erikj/8180651/webrev.01/

/Erik

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems


On 19/05/2017 7:25 PM, Robbin Ehn wrote:

On 05/19/2017 11:07 AM, David Holmes wrote:


They have to be as there are three cases:

1. Relative wait using CLOCK_MONOTONIC
2. Relative wait using gettimeofday()
3. Absolute wait using gettimeofday()


Please consider something like:

#ifdef SUPPORTS_CLOCK_MONOTONIC
   if (_use_clock_monotonic_condattr  && !isAbsolute) { // Why aren't 
we using this when not isAbsolute is set?
// I suggest removing that check from 
this if and use monotonic for that also.


Absolute waits have to be based on wall-clock time and follow any 
adjustments made to wall clock time. In contrast relative waits should 
never be affected by wall-clock time adjustments hence the use of 
CLOCK_MONOTONIC when available.


In Java the relative timed-waits are:
- Thread.sleep(ms)
- Object.wait(ms)/wait(ms,ns)
- LockSupport.parkNanos(ns) (and all the j.u.c blocking methods built 
on top of it)


While the only absolute timed-wait we have is the 
LockSupport.parkUntil method(s).


Hope that clarifies things.


Yes thanks!

But you can still re-factoring to something similar to what I suggested 
and two of the calculation should be the same just ns vs us, correct?


There are three different forms of the calculation. The two relative 
time versions use a different time function and so a different time 
structure (timeval vs timespec) and a different calculation.


Leaving the if statement with the "!isAbsolute" check, in my head 
calc_time is something like:


void calc_time(...) {
   if (isAbsolute) {
 calc_abs_time(...);
   } else {

#ifdef SUPPORTS_CLOCK_MONOTONIC

 calc_rel_time_from_clock_monotonic(...);

#else
>  calc_rel_time_from_gettimeofday(...);
#endif

   }
}


David
-


I do not see a problem with this, only better readability?

/Robbin



Thanks,
David
-


  struct timespec now;
  int status = _clock_gettime(CLOCK_MONOTONIC, );
  assert_status(status == 0, status, "clock_gettime");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, 
now.tv_nsec, NANOUNITS);

   } else {
#else
   {
#endif
  struct timeval now;
  int status = gettimeofday(, NULL);
  assert(status == 0, "gettimeofday");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, 
now.tv_usec, MICROUNITS);

   }
#endif

Thanks for fixing this!

/Robbin

Re: RFR: JDK-8180651: Make target to run tests on exploded image

I like the idea, the changes in itself look good, but I really *really* 
do not like the name "exploded". It was not good before as in 
"exploded-image", but this is even worse. :-(


Can we pause this one for just a while and try really hard to come up 
with a better name? If we fail to do that in a couple of days, I'll 
admit defeat and accept the patch as it is. OK?


What about "local" image? "simple"? "quick"? "bare"?

/Magnus

On 2017-05-19 11:26, Erik Joelsson wrote:
In most cases, when running regression tests, you want to run them on 
the jdk image because that's the correct image and some tests will not 
work on other variants. However, many tests can be run successfully on 
the exploded image and since building the full jdk image takes quite a 
bit of extra time, especially when running a slowdebug build, enabling 
this as an option can significantly improve developer efficiency.


I therefor propose the new target "exploded-run-test". It will work 
just like run-test but it depends on and runs on the exploded image.


Bug: https://bugs.openjdk.java.net/browse/JDK-8180651

Webrev: http://cr.openjdk.java.net/~erikj/8180651/webrev.01/

/Erik

Re: RFR: JDK-8180651: Make target to run tests on exploded image


Thank you Erik for doing this!

/Robbin

On 05/19/2017 11:26 AM, Erik Joelsson wrote:
In most cases, when running regression tests, you want to run them on the jdk image because that's the correct image and some tests will not work on other variants. 
However, many tests can be run successfully on the exploded image and since building the full jdk image takes quite a bit of extra time, especially when running a slowdebug 
build, enabling this as an option can significantly improve developer efficiency.


I therefor propose the new target "exploded-run-test". It will work just like 
run-test but it depends on and runs on the exploded image.

Bug: https://bugs.openjdk.java.net/browse/JDK-8180651

Webrev: http://cr.openjdk.java.net/~erikj/8180651/webrev.01/

/Erik

RFR: JDK-8175824 Adapt javadoc generation to different requirements for JDK and JavaSE

javadoc allows a file to be specified to provide content for the top
level "overview" page.

As we move towards a unified docs bundle, we need to be able to vary the
content of that file depending on the content of the bundle. This does
not mean providing or including lists of modules, but should reflect
whether the bundle contains Java SE modules, JDK modules and JavaFX
modules.

This patch also includes support for (and relies on) the new usage of
the -group option from JDK-8180336.

Note that when JDK-8180480 (Use "requires transitive" relationship when
determining modules for javadoc) is pushed, the call to
FindTransitiveDepsForModules for JavaSE_MODULES should be updated to the
new FindTransitiveIndirectExportsForModules (or whatever it'll end up
being called). I'll fix that in whichever of the two bugs I push last.

This patch also contains some accumulated cleanup in Docs.gmk after all
the piecemeal patches this file has recieved for the last few weeks.

Note: While I'm willing to make minor changes to the actual contents of
the generated overview.html, I'd prefer if any larger (or potentially
controversial) issues are handled as separate follow-up bugs. With this
framework in place, it's easy to modify whatever gets written to the file.

An example how this looks like for the JDK and Java SE documentation
collections (stripped down to the summary page of the javadoc output,
and the JDK top-level index.html) is here:

http://cr.openjdk.java.net/~ihse/demo-generated-overview/

Bug: https://bugs.openjdk.java.net/browse/JDK-8175824
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8175824-javadoc-dynamic-overview-and-groups/webrev.01

RFR: JDK-8180651: Make target to run tests on exploded image

In most cases, when running regression tests, you want to run them on 
the jdk image because that's the correct image and some tests will not 
work on other variants. However, many tests can be run successfully on 
the exploded image and since building the full jdk image takes quite a 
bit of extra time, especially when running a slowdebug build, enabling 
this as an option can significantly improve developer efficiency.


I therefor propose the new target "exploded-run-test". It will work just 
like run-test but it depends on and runs on the exploded image.


Bug: https://bugs.openjdk.java.net/browse/JDK-8180651

Webrev: http://cr.openjdk.java.net/~erikj/8180651/webrev.01/

/Erik

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems


On 05/19/2017 11:07 AM, David Holmes wrote:


They have to be as there are three cases:

1. Relative wait using CLOCK_MONOTONIC
2. Relative wait using gettimeofday()
3. Absolute wait using gettimeofday()


Please consider something like:

#ifdef SUPPORTS_CLOCK_MONOTONIC
   if (_use_clock_monotonic_condattr  && !isAbsolute) { // Why aren't we using 
this when not isAbsolute is set?
// I suggest removing that check from this if 
and use monotonic for that also.


Absolute waits have to be based on wall-clock time and follow any adjustments made to wall clock time. In contrast relative waits should never be affected by wall-clock 
time adjustments hence the use of CLOCK_MONOTONIC when available.


In Java the relative timed-waits are:
- Thread.sleep(ms)
- Object.wait(ms)/wait(ms,ns)
- LockSupport.parkNanos(ns) (and all the j.u.c blocking methods built on top of 
it)

While the only absolute timed-wait we have is the LockSupport.parkUntil 
method(s).

Hope that clarifies things.


Yes thanks!

But you can still re-factoring to something similar to what I suggested and two 
of the calculation should be the same just ns vs us, correct?
Leaving the if statement with the "!isAbsolute" check, in my head calc_time is 
something like:

void calc_time(...) {
  if (isAbsolute) {
calc_abs_time(...);
  } else {
calc_rel_time(...);
  }
}

I do not see a problem with this, only better readability?

/Robbin



Thanks,
David
-


  struct timespec now;
  int status = _clock_gettime(CLOCK_MONOTONIC, );
  assert_status(status == 0, status, "clock_gettime");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_nsec, 
NANOUNITS);
   } else {
#else
   {
#endif
  struct timeval now;
  int status = gettimeofday(, NULL);
  assert(status == 0, "gettimeofday");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_usec, 
MICROUNITS);
   }
#endif

Thanks for fixing this!

/Robbin

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems

2017-05-19 Thread Doug Simon


> On 19 May 2017, at 11:15, Magnus Ihse Bursie  
> wrote:
> 
> 
> On 2017-05-19 09:15, David Holmes wrote:
>> Hi Magnus,
>> 
>> On 18/05/2017 8:06 PM, Magnus Ihse Bursie wrote:
>>> 
>>> 
>>> On 2017-05-18 09:35, David Holmes wrote:
 On 18/05/2017 5:32 PM, Magnus Ihse Bursie wrote:
> On 2017-05-18 08:25, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8174231
>> 
>> webrevs:
>> 
>> Build-related: http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/
> 
> Build changes look  good.
 
 Thanks Magnus! I just realized I left in the AC_MSG_NOTICE debugging 
 prints outs - do you want me to remove them? I suppose they may be useful 
 if something goes wrong on some platform.
>>> 
>>> I didn't even notice them. :-/
>>> 
>>> It's a bit unfortunate we don't have a debug level on the logging from 
>>> configure. :-( Otherwise they would have clearly belonged there.
>>> 
>>> The AC_MSG_NOTICE messages stands out much from the rest of the configure 
>>> log, so maybe it's better that you remove them. The logic itself is very 
>>> simple, if the -D flags are missing then we can surely tell what happened. 
>>> So yes, please remove them.
>> 
>> Webrev updated in place.
> Code looks good!
> 
> In the future, I very much prefer if you do not update webrevs in place. It's 
> hopeless if you start reading a thread after some updates have occured, the 
> mails don't make any sense, and it's hard to follow after-the-fact how the 
> patch evolved.

Is there any chance openjdk code reviewing will adopt a slightly more modern 
process than webrevs such as Crucible where a full history of code evolution 
during a review is preserved?

-Doug

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems


Replying to all this time :)

On 19/05/2017 7:15 PM, Magnus Ihse Bursie wrote:


On 2017-05-19 09:15, David Holmes wrote:

Hi Magnus,

On 18/05/2017 8:06 PM, Magnus Ihse Bursie wrote:



On 2017-05-18 09:35, David Holmes wrote:

On 18/05/2017 5:32 PM, Magnus Ihse Bursie wrote:

On 2017-05-18 08:25, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8174231

webrevs:

Build-related: 
http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/


Build changes look  good.


Thanks Magnus! I just realized I left in the AC_MSG_NOTICE debugging 
prints outs - do you want me to remove them? I suppose they may be 
useful if something goes wrong on some platform.


I didn't even notice them. :-/

It's a bit unfortunate we don't have a debug level on the logging 
from configure. :-( Otherwise they would have clearly belonged there.


The AC_MSG_NOTICE messages stands out much from the rest of the 
configure log, so maybe it's better that you remove them. The logic 
itself is very simple, if the -D flags are missing then we can surely 
tell what happened. So yes, please remove them.


Webrev updated in place.

Code looks good!


Thanks for the re-review.

In the future, I very much prefer if you do not update webrevs in place. 
It's hopeless if you start reading a thread after some updates have 
occured, the mails don't make any sense, and it's hard to follow 
after-the-fact how the patch evolved.


Sorry. Point taken.

David
-




I have removed them to avoid noise - particularly as they get executed 
twice.


I also made an adjustment to AC_SEARCH_LIBS as I don't want to pass 
the saved_LIBS value in explicitly, but want it to use the LIBS value 
- which is no longer cleared before the call. I've verified all 
platforms are okay - except AIX which I'll need Thomas to recheck when 
he can.


I also discovered an oddity in that our ARM64 builds seem to use 
different system libraries in that librt.so is not needed for 
clock_gettime. This still seems to work ok. Of more of a concern if we 
were expand this kind of function-existence check is that libc seems 
to contain "dummy" (or at least dysfunctional) versions of a number of 
the core pthread APIs!

That's good to know.

/Magnus


Thanks,
David

Alternatively, rewrite them as CHECKING/RESULT, if you want to keep 
the logging. That way they match better the rest of the configure log 
(and also describes what you're doing). Just check if AC_SEARCH_LIBS 
prints some output (likely so, I think), then you can't do it in the 
middle of a CHECKING/RESULT pair, but have to do the CHECKING part 
after AC_SEARCH_LIBS.


/Magnus



David


/Magnus


  hotspot:
http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot/

First a big thank you to Thomas Stuefe for testing various 
versions of

this on AIX.

This is primarily a refactoring and cleanup exercise (ie lots of
deleted duplicated code!).

I have taken the PlatformEvent, PlatformParker and Parker::* code, 
out
of os_linux and moved it into os_posix for use by Linux, OSX, BSD, 
AIX

and perhaps one day Solaris (more on that later).

The Linux code was the most functionally complete, dealing with
correct use of CLOCK_MONOTONIC for relative timed waits, and the
default wall-clock for absolute timed waits. That functionality is
not, unfortunately, supported by all our POSIX platforms so there are
some configure time build checks to set some #defines, and then some
dynamic lookup at runtime**. We allow for the runtime environment to
be less capable than the build environment, but not the other way
around (without build time support we don't know the runtime types
needed to make library calls).

** There is some duplication of dynamic lookup code on Linux but this
can be cleaned up in future work if we refactor the time/clock code
into os_posix as well.

The cleanup covers a number of things:
- removal of linux anachronisms that got "ported" into the other
platforms
  - eg EINTR can not be returned from the wait methods
- removal of solaris anachronisms that got ported into the linux code
and then on to other platforms
  - eg ETIMEDOUT is what we expect never ETIME
- removal of the ancient/obsolete os::*::allowdebug_blocked_signals()
from the Parker methods
- consolidation of unpackTime and compute_abstime into one utility
function
- use statics for things completely private to the implementation
rather than making them part of the os* API (eg access to condAttr
objects)
- cleanup up commentary and style within methods of the same class
- clean up coding style in places eg not using Names that start with
capitals.

I have not tried to cleanup every single oddity, nor tried to
reconcile differences between the very similar in places 
PlatformEvent

and Park methods. For example PlatformEvent still examines the
FilterSpuriousWakeups** flag, and Parker still ignores it.

** Perhaps a candidate for deprecation and future removal.

There is one mini "enhancement" slipped in this. I now explicitly
initialize

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems



On 2017-05-19 09:15, David Holmes wrote:

Hi Magnus,

On 18/05/2017 8:06 PM, Magnus Ihse Bursie wrote:



On 2017-05-18 09:35, David Holmes wrote:

On 18/05/2017 5:32 PM, Magnus Ihse Bursie wrote:

On 2017-05-18 08:25, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8174231

webrevs:

Build-related: 
http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/


Build changes look  good.


Thanks Magnus! I just realized I left in the AC_MSG_NOTICE debugging 
prints outs - do you want me to remove them? I suppose they may be 
useful if something goes wrong on some platform.


I didn't even notice them. :-/

It's a bit unfortunate we don't have a debug level on the logging 
from configure. :-( Otherwise they would have clearly belonged there.


The AC_MSG_NOTICE messages stands out much from the rest of the 
configure log, so maybe it's better that you remove them. The logic 
itself is very simple, if the -D flags are missing then we can surely 
tell what happened. So yes, please remove them.


Webrev updated in place.

Code looks good!

In the future, I very much prefer if you do not update webrevs in place. 
It's hopeless if you start reading a thread after some updates have 
occured, the mails don't make any sense, and it's hard to follow 
after-the-fact how the patch evolved.




I have removed them to avoid noise - particularly as they get executed 
twice.


I also made an adjustment to AC_SEARCH_LIBS as I don't want to pass 
the saved_LIBS value in explicitly, but want it to use the LIBS value 
- which is no longer cleared before the call. I've verified all 
platforms are okay - except AIX which I'll need Thomas to recheck when 
he can.


I also discovered an oddity in that our ARM64 builds seem to use 
different system libraries in that librt.so is not needed for 
clock_gettime. This still seems to work ok. Of more of a concern if we 
were expand this kind of function-existence check is that libc seems 
to contain "dummy" (or at least dysfunctional) versions of a number of 
the core pthread APIs!

That's good to know.

/Magnus


Thanks,
David

Alternatively, rewrite them as CHECKING/RESULT, if you want to keep 
the logging. That way they match better the rest of the configure log 
(and also describes what you're doing). Just check if AC_SEARCH_LIBS 
prints some output (likely so, I think), then you can't do it in the 
middle of a CHECKING/RESULT pair, but have to do the CHECKING part 
after AC_SEARCH_LIBS.


/Magnus



David


/Magnus


  hotspot:
http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot/

First a big thank you to Thomas Stuefe for testing various 
versions of

this on AIX.

This is primarily a refactoring and cleanup exercise (ie lots of
deleted duplicated code!).

I have taken the PlatformEvent, PlatformParker and Parker::* code, 
out
of os_linux and moved it into os_posix for use by Linux, OSX, BSD, 
AIX

and perhaps one day Solaris (more on that later).

The Linux code was the most functionally complete, dealing with
correct use of CLOCK_MONOTONIC for relative timed waits, and the
default wall-clock for absolute timed waits. That functionality is
not, unfortunately, supported by all our POSIX platforms so there are
some configure time build checks to set some #defines, and then some
dynamic lookup at runtime**. We allow for the runtime environment to
be less capable than the build environment, but not the other way
around (without build time support we don't know the runtime types
needed to make library calls).

** There is some duplication of dynamic lookup code on Linux but this
can be cleaned up in future work if we refactor the time/clock code
into os_posix as well.

The cleanup covers a number of things:
- removal of linux anachronisms that got "ported" into the other
platforms
  - eg EINTR can not be returned from the wait methods
- removal of solaris anachronisms that got ported into the linux code
and then on to other platforms
  - eg ETIMEDOUT is what we expect never ETIME
- removal of the ancient/obsolete os::*::allowdebug_blocked_signals()
from the Parker methods
- consolidation of unpackTime and compute_abstime into one utility
function
- use statics for things completely private to the implementation
rather than making them part of the os* API (eg access to condAttr
objects)
- cleanup up commentary and style within methods of the same class
- clean up coding style in places eg not using Names that start with
capitals.

I have not tried to cleanup every single oddity, nor tried to
reconcile differences between the very similar in places 
PlatformEvent

and Park methods. For example PlatformEvent still examines the
FilterSpuriousWakeups** flag, and Parker still ignores it.

** Perhaps a candidate for deprecation and future removal.

There is one mini "enhancement" slipped in this. I now explicitly
initialize mutexes with a mutexAttr object with its type set to
PTHREAD_MUTEX_NORMAL, instead of relying on the definition of
PTHREAD_MUTEX_DEFAULT. On

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems


Hi Robbin,

Thanks for looking at this.

On 19/05/2017 6:36 PM, Robbin Ehn wrote:

Hi David,

On 05/18/2017 08:25 AM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8174231

webrevs:

Build-related: http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/
   hotspot: 
http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot/




I like this, with neg delta of 700 loc, nice!

It's hard to see if you broken anything, since you combined 4 separate 
implementation into 1.


Well not really. As I said this is basically a cleaned up version of the 
Linux code. The BSD and AIX versions were already based on earlier 
versions of the Linux code, minus the proper handling of CLOCK_MONOTONIC 
and absolute timeouts.



I guess you have tested this proper?


Only JPRT so far. I should have mentioned that I'm not expecting this to 
be reviewed in pushed within a couple of days, so some refinements and 
continual testing will occur.



What stands out in os_posix.cpp is the
static void to_abstime(timespec* abstime, jlong timeout, bool isAbsolute)

The ifdef scopes of SUPPORTS_CLOCK_MONOTONIC is large and calculations 
are repeated 3 times.


They have to be as there are three cases:

1. Relative wait using CLOCK_MONOTONIC
2. Relative wait using gettimeofday()
3. Absolute wait using gettimeofday()


Please consider something like:

#ifdef SUPPORTS_CLOCK_MONOTONIC
   if (_use_clock_monotonic_condattr  && !isAbsolute) { // Why aren't we 
using this when not isAbsolute is set?
// I suggest removing that check from 
this if and use monotonic for that also.


Absolute waits have to be based on wall-clock time and follow any 
adjustments made to wall clock time. In contrast relative waits should 
never be affected by wall-clock time adjustments hence the use of 
CLOCK_MONOTONIC when available.


In Java the relative timed-waits are:
- Thread.sleep(ms)
- Object.wait(ms)/wait(ms,ns)
- LockSupport.parkNanos(ns) (and all the j.u.c blocking methods built on 
top of it)


While the only absolute timed-wait we have is the LockSupport.parkUntil 
method(s).


Hope that clarifies things.

Thanks,
David
-


  struct timespec now;
  int status = _clock_gettime(CLOCK_MONOTONIC, );
  assert_status(status == 0, status, "clock_gettime");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_nsec, 
NANOUNITS);

   } else {
#else
   {
#endif
  struct timeval now;
  int status = gettimeofday(, NULL);
  assert(status == 0, "gettimeofday");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_usec, 
MICROUNITS);

   }
#endif

Thanks for fixing this!

/Robbin

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems


Thanks Erik!

David

On 19/05/2017 6:07 PM, Erik Joelsson wrote:

Build changes look good to me.

/Erik


On 2017-05-19 09:15, David Holmes wrote:

Hi Magnus,

On 18/05/2017 8:06 PM, Magnus Ihse Bursie wrote:



On 2017-05-18 09:35, David Holmes wrote:

On 18/05/2017 5:32 PM, Magnus Ihse Bursie wrote:

On 2017-05-18 08:25, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8174231

webrevs:

Build-related: 
http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/


Build changes look  good.


Thanks Magnus! I just realized I left in the AC_MSG_NOTICE debugging 
prints outs - do you want me to remove them? I suppose they may be 
useful if something goes wrong on some platform.


I didn't even notice them. :-/

It's a bit unfortunate we don't have a debug level on the logging 
from configure. :-( Otherwise they would have clearly belonged there.


The AC_MSG_NOTICE messages stands out much from the rest of the 
configure log, so maybe it's better that you remove them. The logic 
itself is very simple, if the -D flags are missing then we can surely 
tell what happened. So yes, please remove them.


Webrev updated in place.

I have removed them to avoid noise - particularly as they get executed 
twice.


I also made an adjustment to AC_SEARCH_LIBS as I don't want to pass 
the saved_LIBS value in explicitly, but want it to use the LIBS value 
- which is no longer cleared before the call. I've verified all 
platforms are okay - except AIX which I'll need Thomas to recheck when 
he can.


I also discovered an oddity in that our ARM64 builds seem to use 
different system libraries in that librt.so is not needed for 
clock_gettime. This still seems to work ok. Of more of a concern if we 
were expand this kind of function-existence check is that libc seems 
to contain "dummy" (or at least dysfunctional) versions of a number of 
the core pthread APIs!


Thanks,
David

Alternatively, rewrite them as CHECKING/RESULT, if you want to keep 
the logging. That way they match better the rest of the configure log 
(and also describes what you're doing). Just check if AC_SEARCH_LIBS 
prints some output (likely so, I think), then you can't do it in the 
middle of a CHECKING/RESULT pair, but have to do the CHECKING part 
after AC_SEARCH_LIBS.


/Magnus



David


/Magnus


  hotspot:
http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot/

First a big thank you to Thomas Stuefe for testing various 
versions of

this on AIX.

This is primarily a refactoring and cleanup exercise (ie lots of
deleted duplicated code!).

I have taken the PlatformEvent, PlatformParker and Parker::* code, 
out
of os_linux and moved it into os_posix for use by Linux, OSX, BSD, 
AIX

and perhaps one day Solaris (more on that later).

The Linux code was the most functionally complete, dealing with
correct use of CLOCK_MONOTONIC for relative timed waits, and the
default wall-clock for absolute timed waits. That functionality is
not, unfortunately, supported by all our POSIX platforms so there are
some configure time build checks to set some #defines, and then some
dynamic lookup at runtime**. We allow for the runtime environment to
be less capable than the build environment, but not the other way
around (without build time support we don't know the runtime types
needed to make library calls).

** There is some duplication of dynamic lookup code on Linux but this
can be cleaned up in future work if we refactor the time/clock code
into os_posix as well.

The cleanup covers a number of things:
- removal of linux anachronisms that got "ported" into the other
platforms
  - eg EINTR can not be returned from the wait methods
- removal of solaris anachronisms that got ported into the linux code
and then on to other platforms
  - eg ETIMEDOUT is what we expect never ETIME
- removal of the ancient/obsolete os::*::allowdebug_blocked_signals()
from the Parker methods
- consolidation of unpackTime and compute_abstime into one utility
function
- use statics for things completely private to the implementation
rather than making them part of the os* API (eg access to condAttr
objects)
- cleanup up commentary and style within methods of the same class
- clean up coding style in places eg not using Names that start with
capitals.

I have not tried to cleanup every single oddity, nor tried to
reconcile differences between the very similar in places 
PlatformEvent

and Park methods. For example PlatformEvent still examines the
FilterSpuriousWakeups** flag, and Parker still ignores it.

** Perhaps a candidate for deprecation and future removal.

There is one mini "enhancement" slipped in this. I now explicitly
initialize mutexes with a mutexAttr object with its type set to
PTHREAD_MUTEX_NORMAL, instead of relying on the definition of
PTHREAD_MUTEX_DEFAULT. On FreesBSD the default is not "normal" but
"error checking" and so is slow. On all other current platforms there
is no effective change.

Finally, Solaris is excluded from all this (other than the

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems


Hi David,

On 05/18/2017 08:25 AM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8174231

webrevs:

Build-related: http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/
   hotspot: http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot/



I like this, with neg delta of 700 loc, nice!

It's hard to see if you broken anything, since you combined 4 separate 
implementation into 1.
I guess you have tested this proper?

What stands out in os_posix.cpp is the
static void to_abstime(timespec* abstime, jlong timeout, bool isAbsolute)

The ifdef scopes of SUPPORTS_CLOCK_MONOTONIC is large and calculations are 
repeated 3 times.

Please consider something like:

#ifdef SUPPORTS_CLOCK_MONOTONIC
  if (_use_clock_monotonic_condattr  && !isAbsolute) { // Why aren't we using 
this when not isAbsolute is set?
   // I suggest removing 
that check from this if and use monotonic for that also.
 struct timespec now;
 int status = _clock_gettime(CLOCK_MONOTONIC, );
 assert_status(status == 0, status, "clock_gettime");
 calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_nsec, 
NANOUNITS);
  } else {
#else
  {
#endif
 struct timeval now;
 int status = gettimeofday(, NULL);
 assert(status == 0, "gettimeofday");
 calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_usec, 
MICROUNITS);
  }
#endif

Thanks for fixing this!

/Robbin

Re: RFR: JDK-8180480 Use "requires transitive" relationship when determining modules for javadoc

Looks good.

/Erik

On 2017-05-19 10:08, Magnus Ihse Bursie wrote:

On 2017-05-18 17:30, Mandy Chung wrote:
On May 18, 2017, at 12:54 AM, Magnus Ihse Bursie
wrote:

When the build system tries to figure out which modules that should
be included by the javadoc build, it locates the set of modules
"required" by already included modules, starting from an initial set
and repeating recursively (a method which we unfortunately called
"transitive”).

Which module has this name clash?

However, for javadoc, we should only look at those re-exported
modules that are classified by a "requires transitive" relationship
(which I therefore have called "re-exported" to not clash with the
established term "transitive”).
Javadoc uses the “Indirect Exports” term for the exported API
packages are from the “requires transitive” modules.

"Indirect exports" was a good term. I dropped the "Deps" part of the
name and it all became much clearer.

Here's an updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8180480-docs-should-use-requires-transitive/webrev.02

/Magnus

I suggest to rename “ReExported” to “IndirectRequires” or “Indirect”.

Mandy

The effect of getting this wrong is e.g. that we copy module graph
png files that we should not, and that we look at too broad a set of
files for looking at changes that should trigger a re-build of the
javadoc.

Bug: https://bugs.openjdk.java.net/browse/JDK-8180480
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8180480-docs-should-use-requires-transitive/webrev.01

/Magnus

Re: RFR: JDK-8180480 Use "requires transitive" relationship when determining modules for javadoc




On 2017-05-18 17:30, Mandy Chung wrote:

On May 18, 2017, at 12:54 AM, Magnus Ihse Bursie 
 wrote:

When the build system tries to figure out which modules that should be included by the javadoc 
build, it locates the set of modules "required" by already included modules, 
starting from an initial set and repeating recursively (a method which we unfortunately called 
"transitive”).

Which module has this name clash?


However, for javadoc, we should only look at those re-exported modules that are classified by a 
"requires transitive" relationship (which I therefore have called "re-exported" to 
not clash with the established term "transitive”).

Javadoc uses the “Indirect Exports” term for the exported API packages are from 
the “requires transitive” modules.


"Indirect exports" was a good term. I dropped the "Deps" part of the 
name and it all became much clearer.


Here's an updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8180480-docs-should-use-requires-transitive/webrev.02

/Magnus



I suggest to rename “ReExported” to “IndirectRequires” or “Indirect”.

Mandy


The effect of getting this wrong is e.g. that we copy module graph png files 
that we should not, and that we look at too broad a set of files for looking at 
changes that should trigger a re-build of the javadoc.

Bug: https://bugs.openjdk.java.net/browse/JDK-8180480
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8180480-docs-should-use-requires-transitive/webrev.01

/Magnus

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems


Build changes look good to me.

/Erik


On 2017-05-19 09:15, David Holmes wrote:

Hi Magnus,

On 18/05/2017 8:06 PM, Magnus Ihse Bursie wrote:



On 2017-05-18 09:35, David Holmes wrote:

On 18/05/2017 5:32 PM, Magnus Ihse Bursie wrote:

On 2017-05-18 08:25, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8174231

webrevs:

Build-related: 
http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/


Build changes look  good.


Thanks Magnus! I just realized I left in the AC_MSG_NOTICE debugging 
prints outs - do you want me to remove them? I suppose they may be 
useful if something goes wrong on some platform.


I didn't even notice them. :-/

It's a bit unfortunate we don't have a debug level on the logging 
from configure. :-( Otherwise they would have clearly belonged there.


The AC_MSG_NOTICE messages stands out much from the rest of the 
configure log, so maybe it's better that you remove them. The logic 
itself is very simple, if the -D flags are missing then we can surely 
tell what happened. So yes, please remove them.


Webrev updated in place.

I have removed them to avoid noise - particularly as they get executed 
twice.


I also made an adjustment to AC_SEARCH_LIBS as I don't want to pass 
the saved_LIBS value in explicitly, but want it to use the LIBS value 
- which is no longer cleared before the call. I've verified all 
platforms are okay - except AIX which I'll need Thomas to recheck when 
he can.


I also discovered an oddity in that our ARM64 builds seem to use 
different system libraries in that librt.so is not needed for 
clock_gettime. This still seems to work ok. Of more of a concern if we 
were expand this kind of function-existence check is that libc seems 
to contain "dummy" (or at least dysfunctional) versions of a number of 
the core pthread APIs!


Thanks,
David

Alternatively, rewrite them as CHECKING/RESULT, if you want to keep 
the logging. That way they match better the rest of the configure log 
(and also describes what you're doing). Just check if AC_SEARCH_LIBS 
prints some output (likely so, I think), then you can't do it in the 
middle of a CHECKING/RESULT pair, but have to do the CHECKING part 
after AC_SEARCH_LIBS.


/Magnus



David


/Magnus


  hotspot:
http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot/

First a big thank you to Thomas Stuefe for testing various 
versions of

this on AIX.

This is primarily a refactoring and cleanup exercise (ie lots of
deleted duplicated code!).

I have taken the PlatformEvent, PlatformParker and Parker::* code, 
out
of os_linux and moved it into os_posix for use by Linux, OSX, BSD, 
AIX

and perhaps one day Solaris (more on that later).

The Linux code was the most functionally complete, dealing with
correct use of CLOCK_MONOTONIC for relative timed waits, and the
default wall-clock for absolute timed waits. That functionality is
not, unfortunately, supported by all our POSIX platforms so there are
some configure time build checks to set some #defines, and then some
dynamic lookup at runtime**. We allow for the runtime environment to
be less capable than the build environment, but not the other way
around (without build time support we don't know the runtime types
needed to make library calls).

** There is some duplication of dynamic lookup code on Linux but this
can be cleaned up in future work if we refactor the time/clock code
into os_posix as well.

The cleanup covers a number of things:
- removal of linux anachronisms that got "ported" into the other
platforms
  - eg EINTR can not be returned from the wait methods
- removal of solaris anachronisms that got ported into the linux code
and then on to other platforms
  - eg ETIMEDOUT is what we expect never ETIME
- removal of the ancient/obsolete os::*::allowdebug_blocked_signals()
from the Parker methods
- consolidation of unpackTime and compute_abstime into one utility
function
- use statics for things completely private to the implementation
rather than making them part of the os* API (eg access to condAttr
objects)
- cleanup up commentary and style within methods of the same class
- clean up coding style in places eg not using Names that start with
capitals.

I have not tried to cleanup every single oddity, nor tried to
reconcile differences between the very similar in places 
PlatformEvent

and Park methods. For example PlatformEvent still examines the
FilterSpuriousWakeups** flag, and Parker still ignores it.

** Perhaps a candidate for deprecation and future removal.

There is one mini "enhancement" slipped in this. I now explicitly
initialize mutexes with a mutexAttr object with its type set to
PTHREAD_MUTEX_NORMAL, instead of relying on the definition of
PTHREAD_MUTEX_DEFAULT. On FreesBSD the default is not "normal" but
"error checking" and so is slow. On all other current platforms there
is no effective change.

Finally, Solaris is excluded from all this (other than the debug
signal blocking cleanup) because it potentially supports

Re: CR: 8180540: Add pandoc build fix for windows

Hello,

On 2017-05-18 19:10, Brad R. Wetmore wrote:

On 5/18/2017 12:27 AM, Magnus Ihse Bursie wrote:

Looks good. Formally, I believe someone else needs to review it.

Hm...I would have expected your "Contributed-by" and my review would
be sufficient (what we do for sponsoring an "author" change), but
looks like Erik did review also so we should be fine.

Erik wrote:

> As long as pandoc for windows is always a windows native thing and
> not provided by cygwin this seems good. A quick googling indicates
> that cygwin currently does not provide pandoc so should be fine then.

You are correct in that pandoc is not provded by cygwin, I had to
download/install cygdrive separately (grrr...), then set the PANDOC
configure option:

bash configure \
PANDOC=c:/Users/mydir/bin/pandoc.exe \

That said, I'm not quite following your point about cygwin's
noninclusion of pandoc here.

The underlying issue is that the arguments being passed to pandoc
aren't being processed correctly.

...deleted...
--toc
'/cygdrive/d/java/ws/dev/jdk/src/closed/java.base/share/specs/security/standard-names.md'
-o
'/cygdrive/d/java/ws/dev/build/windows-x86_64-normal-server-release/support/javadoc/specs/security/standard-names.html'

pandoc.exe:
/cygdrive/d/java/ws/dev/jdk/src/closed/java.base/share/specs/security/standard-names.md:
openFile: does not exist (No such file or directory)

Changing to the following worked:

--toc
'd:/java/ws/dev/jdk/src/closed/java.base/share/specs/security/standard-names.md'
-o
'd:/java/ws/dev/build/windows-x86_64-normal-server-release/support/javadoc/specs/security/standard-names.html'

which is what FIXPATH does, IIUC.

That is indeed what fixpath does, but if pandoc was a cygwin native app,
it would accept /cygdrive/x paths. It would probably work fine with x:/
paths as well, but in some cases those windows paths do weird things.

/Erik

Thanks for the review.

Brad

/Magnus

On 2017-05-18 02:07, Brad R. Wetmore wrote:

Magnus,

I've added your suggested fix to spec.gmk.in, which is the minor tweak
to add @FIXPATH@ to allow pandoc to run on windows builds.

https://bugs.openjdk.java.net/browse/JDK-8180540
http://cr.openjdk.java.net/~wetmore/8180540/webrev.00/

I put you down as Contributed-by: with myself as reviewer, but thought
you (and others?) might want to at least check it.

Thanks,

Brad

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems