Re: CMake replacing Autotools?

[Robin Westberg]

Hi Christoph,

> On 19 Mar 2021, at 00:00, Christoph Grüninger  wrote:
> 
> ...
> 
> 2. More choices to actually build the project: Use integrated build
> tools of IDEs (Visual Studio, Xcode) or use Ninja, which is faster than
> gmake
> 
> ...
> 
> 4. CMake is better supported by IDEs like Visual Studio, Qt Creator,
> KDevelop.


Regarding the IDE support points, it’s possible to generate a CMakeLists.txt 
from the compile_commands.json file created when building using the current 
make system. You could then use CMake to generate a native project file for 
your IDE of choice, and use that for compiling and debugging HotSpot (final 
linking etc would still be done by make). I have a prototype for this that 
worked reasonably well with at least Xcode, CLion and Visual Studio as I 
remember it. If this sounds interesting to anyone I could perhaps try to make 
it available somewhere.. :)

Best regards,
Robin

> 
> 5. A lot of code bases were ported to CMake like KDE, Qt, or LLVM. Their
> arguments apply here, too. Also their trade-offs between investment and
> benefit.
> 
>> At various times, I have dreamed of replacing the configure script with
>> something that is more modern and easy to maintain than this bash/m4
>> mix. We have a very well-defined API for the configure script: the user
>> calls "bash configure" in the root directory of the project, with a set
>> of --options, and as a result we create a spec.gmk file that defines the
>> configuration. This could easily be replicated in another system. But if
>> I were to rewrite this from scratch, I'd rather write the whole
>> configure logic in Java (apart from some thin shell script logic needed
>> to find the boot jdk), rather than trying to shoehorn in our build model
>> in CMake.
> 
> I understand your temptation, but writing and maintaining all the
> configure/find logic and quirks will be a burden. I'd try to reduce the
> build system code to a minimum and rely on a third-party solution to do
> as much as possible for me.
> 
> Bye
> Christoph
> 
> -- 
> Als wär es nix, leb' ich von [IT] und mach' nur, was ich lieb'
> Lebe wie im Paradies, womit hab' ich das verdient?
> Die Wahrheit ist: Hab' ich nicht, ich bin nur reicher beschenkt
> Als jemand in einem armen Land mit dem gleichen Talent
> [frei nach Tua von Die Orsons - Oioioiropa]

Integrated: 8263667: Avoid running GitHub actions on branches named pr/*

[Robin Westberg]

On Tue, 16 Mar 2021 10:53:06 GMT, Robin Westberg  wrote:

> When the Skara feature "dependent pull requests" is activated for the JDK 
> repository, branches with the name "pr/" will start to appear. These 
> will not be synced into personal forks by the Skara sync command, but if they 
> are synced manually, we should avoid running GitHub actions workflows on them.

This pull request has now been integrated.

Changeset: 86e9cd98
Author:Robin Westberg 
URL:   https://git.openjdk.java.net/jdk/commit/86e9cd98
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod

8263667: Avoid running GitHub actions on branches named pr/*

Reviewed-by: ehelin, erikj, ihse

-

PR: https://git.openjdk.java.net/jdk/pull/3024

RFR: 8263667: Avoid running GitHub actions on branches named pr/*

[Robin Westberg]

When the Skara feature "dependent pull requests" is activated for the JDK 
repository, branches with the name "pr/" will start to appear. These 
will not be synced into personal forks by the Skara sync command, but if they 
are synced manually, we should avoid running GitHub actions workflows on them.

-

Commit messages:
 - Filter out branches named pr/ for GitHub actions

Changes: https://git.openjdk.java.net/jdk/pull/3024/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3024=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263667
  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3024.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3024/head:pull/3024

PR: https://git.openjdk.java.net/jdk/pull/3024

Re: RFR: 8258477: Pre-submit testing using GitHub Actions should merge changes from target branch

[Robin Westberg]

On Wed, 16 Dec 2020 10:47:50 GMT, Robin Westberg  wrote:

> Normally when running GitHub Actions on a pull request, what is checked out 
> is the merge of the pull request with the latest changes on the target 
> branch. This ensure that what is tested is as close as possible to what will 
> actually be the result of integrating said pull request. 
> 
> In our use of GitHub Actions, we don't run directly on pull requests but 
> instead allow contributors to run them in their own personal forks. In that 
> context, there is no notion of a target branch. However, we can infer the 
> target project and branch by encoding this in the workflow file. This allows 
> us to perform the same merge manually, and achieve the same behaviour. 
> 
> The change also adds an option to disable this automated merge when launching 
> the workflow manually, which could be useful when investigating unexpected 
> test failures.
> 
> Note that downstream projects picking up this change will have to adapt the 
> workflow file to keep using these actions for pre-submit testing. This has 
> been a common request from downstream projects, but could also be done in a 
> separate change (or not at all).

So, is there anyone who would like to review this? I still think it will 
improve the effectiveness of the GitHub Actions-based testing.

-

PR: https://git.openjdk.java.net/jdk/pull/1801

Re: RFR: 8258477: Pre-submit testing using GitHub Actions should merge changes from target branch

[Robin Westberg]

Hi Thomas,

> On 16 Dec 2020, at 14:54, Thomas Stüfe  wrote:
> 
> Hi Robin,
> 
> does this mean tests won't run on branches which cannot be merged with the
> assumed target branch?

No, if there’s a problem with doing the merge the tests will simply continue 
without doing it, and just use the commit in the pull request as-is.

Best regards,
Robin
 
> 
> Thanks, Thomas
> 
> 
> On Wed, Dec 16, 2020 at 11:55 AM Robin Westberg 
> wrote:
> 
>> Normally when running GitHub Actions on a pull request, what is checked
>> out is the merge of the pull request with the latest changes on the target
>> branch. This ensure that what is tested is as close as possible to what
>> will actually be the result of integrating said pull request.
>> 
>> In our use of GitHub Actions, we don't run directly on pull requests but
>> instead allow contributors to run them in their own personal forks. In that
>> context, there is no notion of a target branch. However, we can infer the
>> target project and branch by encoding this in the workflow file. This
>> allows us to perform the same merge manually, and achieve the same
>> behaviour.
>> 
>> The change also adds an option to disable this automated merge when
>> launching the workflow manually, which could be useful when investigating
>> unexpected test failures.
>> 
>> Note that downstream projects picking up this change will have to adapt
>> the workflow file to keep using these actions for pre-submit testing. This
>> has been a common request from downstream projects, but could also be done
>> in a separate change (or not at all).
>> 
>> -
>> 
>> Commit messages:
>> - Allow opting out of automated merge on manual runs
>> - Initial implementation
>> 
>> Changes: https://git.openjdk.java.net/jdk/pull/1801/files
>> Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1801=00
>>  Issue: https://bugs.openjdk.java.net/browse/JDK-8258477
>>  Stats: 117 lines in 1 file changed: 116 ins; 0 del; 1 mod
>>  Patch: https://git.openjdk.java.net/jdk/pull/1801.diff
>>  Fetch: git fetch https://git.openjdk.java.net/jdk
>> pull/1801/head:pull/1801
>> 
>> PR: https://git.openjdk.java.net/jdk/pull/1801
>> 

RFR: 8258477: Pre-submit testing using GitHub Actions should merge changes from target branch

[Robin Westberg]

Normally when running GitHub Actions on a pull request, what is checked out is 
the merge of the pull request with the latest changes on the target branch. 
This ensure that what is tested is as close as possible to what will actually 
be the result of integrating said pull request. 

In our use of GitHub Actions, we don't run directly on pull requests but 
instead allow contributors to run them in their own personal forks. In that 
context, there is no notion of a target branch. However, we can infer the 
target project and branch by encoding this in the workflow file. This allows us 
to perform the same merge manually, and achieve the same behaviour. 

The change also adds an option to disable this automated merge when launching 
the workflow manually, which could be useful when investigating unexpected test 
failures.

Note that downstream projects picking up this change will have to adapt the 
workflow file to keep using these actions for pre-submit testing. This has been 
a common request from downstream projects, but could also be done in a separate 
change (or not at all).

-

Commit messages:
 - Allow opting out of automated merge on manual runs
 - Initial implementation

Changes: https://git.openjdk.java.net/jdk/pull/1801/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1801=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8258477
  Stats: 117 lines in 1 file changed: 116 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1801.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1801/head:pull/1801

PR: https://git.openjdk.java.net/jdk/pull/1801

Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]

[Robin Westberg]

On Thu, 26 Nov 2020 16:25:35 GMT, Magnus Ihse Bursie  wrote:

>>> That sounds very good indeed! I did not think it was possible for Skara to 
>>> access the Checks area, but if it does, it's really where this belong.
>> 
>> I've tried this out a bit in the playground now - perhaps something like 
>> this: https://github.com/openjdk/playground/pull/77
>
> @rwestberg Looks sooo much better! What happens if a build fails?

I think there were a few failing test jobs in the current state of the 
playground, so should be visible in a little while!

-

PR: https://git.openjdk.java.net/jdk/pull/1350

Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]

[Robin Westberg]

On Mon, 23 Nov 2020 11:54:48 GMT, Magnus Ihse Bursie  wrote:

> That sounds very good indeed! I did not think it was possible for Skara to 
> access the Checks area, but if it does, it's really where this belong.

I've tried this out a bit in the playground now - perhaps something like this: 
https://github.com/openjdk/playground/pull/77

-

PR: https://git.openjdk.java.net/jdk/pull/1350

Re: RFR: 8257056: Submit workflow should apt-get update to avoid package installation errors

[Robin Westberg]

On Wed, 25 Nov 2020 08:56:33 GMT, Aleksey Shipilev  wrote:

> For example, current jobs fail with:
> 
> Get:13 http://azure.archive.ubuntu.com/ubuntu focal/main amd64 libxtst-dev 
> amd64 2:1.2.3-1 [15.2 kB]
> E: Failed to fetch 
> http://azure.archive.ubuntu.com/ubuntu/pool/main/a/alsa-lib/libasound2-dev_1.2.2-2.1ubuntu2.1_amd64.deb
>  404 Not Found [IP: 52.147.219.192 80]
> Fetched 760 kB in 0s (3154 kB/s)
> E: Unable to fetch some archives, maybe run apt-get update or try with 
> --fix-missing?

Looks good!

-

Marked as reviewed by rwestberg (Committer).

PR: https://git.openjdk.java.net/jdk/pull/1427

Re: gh actions fail on linux x64 when fetching libsound

[Robin Westberg]

Hi Aleksey,

> On 25 Nov 2020, at 09:49, Aleksey Shipilev  wrote:
> 
> On 11/25/20 9:45 AM, Thomas Stüfe wrote:
>> "E: Failed to fetch
>> http://azure.archive.ubuntu.com/ubuntu/pool/main/a/alsa-lib/libasound2-dev_1.2.2-2.1ubuntu2.1_amd64.deb
>>  404  Not Found [IP: 52.147.219.192 80]"
>> https://github.com/tstuefe/jdk/runs/1452221121
>> Does anyone have an idea?
> 
> I think this usually means the local aptitude index files are outdated. I 
> suspect we have to do "sudo apt-get update" before "sudo apt-get install". 
> Robin, would you like to do that, or should I?

Yeah I agree, looks like we’re missing an update line for the x64 build (and 
the Linux additional builds too I think). If you want to fix it, feel free. :)

Best regards,
Robin

> 
> -- 
> Thanks,
> -Aleksey
> 

Integrated: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing

[Robin Westberg]

On Fri, 20 Nov 2020 14:24:12 GMT, Robin Westberg  wrote:

> Currently Linux x64 testing in GitHub Actions depends on a few non-relevant 
> hotspot build-only jobs (such as zero) that prevents testing from being run 
> if those build were to fail. As the tests only require the x64 release and 
> debug builds to run and provide interesting feedback, we should separate 
> these other builds into a different job.
> 
> To reduce duplicated steps, all the existing hotspot-only builds have been 
> consolidated into a single job.

This pull request has now been integrated.

Changeset: c45725e5
Author:Robin Westberg 
URL:   https://git.openjdk.java.net/jdk/commit/c45725e5
Stats: 517 lines in 1 file changed: 34 ins; 429 del; 54 mod

8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 
testing

Reviewed-by: shade

-

PR: https://git.openjdk.java.net/jdk/pull/1350

Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]

[Robin Westberg]

On Mon, 23 Nov 2020 09:59:39 GMT, Aleksey Shipilev  wrote:

>> I personally think I prefer to just use a single column for the additional 
>> cross compile builds, because the table becomes very big otherwise (and the 
>> rows get split up). You still see the full name of the build in case 
>> anything fails, so not sure it adds that much value to get an additional box 
>> for each cross-compiled architecture, I guess in 99% of cases they will just 
>> pass and then you don't really care exactly which platforms you didn't 
>> break.. But I'm no UX expert. :)
>
> Yeah, bike-shedding aside, "Linux additional" is probably fine. So if we 
> accept #1379, then it would also add "Windows additional"?

Ah yeah, just saw that one. I guess just one additional column will still make 
the table too wide.. Perhaps all the cross builds should just share a single 
column. :)

Going forward, I do think that the table should perhaps be removed though, and 
the information should instead be moved down to the "checks" box (just as 
GitHub actions show them natively - but perhaps condense them a bit just as the 
current table). The constant updating of the table is probably the major source 
of the "edit war" you have noticed can occur if you want to edit the PR body 
yourself while any GitHub actions are still in progress.

-

PR: https://git.openjdk.java.net/jdk/pull/1350

Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]

[Robin Westberg]

On Fri, 20 Nov 2020 17:31:08 GMT, Aleksey Shipilev  wrote:

>> Robin Westberg has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Improve naming, fix style issues
>
> .github/workflows/submit.yml line 526:
> 
>> 524:   echo "CC=${{ matrix.gnu-arch }}-linux-gnu${{ 
>> matrix.gnu-flavor}}-gcc-10" >> $GITHUB_ENV
>> 525:   echo "CXX=${{ matrix.gnu-arch }}-linux-gnu${{ 
>> matrix.gnu-flavor}}-g++-10" >> $GITHUB_ENV
>> 526:   echo "cross_flags=--openjdk-target=${{ matrix.gnu-arch 
>> }}-linux-gnu${{ matrix.gnu-flavor}} --with-sysroot=${HOME}/sysroot-${{ 
>> matrix.debian-arch }}/ --with-toolchain-path=${HOME}/sysroot-${{ 
>> matrix.debian-arch }}/ --with-freetype-lib=${HOME}/sysroot-${{ 
>> matrix.debian-arch }}/usr/lib/${{ matrix.gnu-arch }}-linux-gnu${{ 
>> matrix.gnu-flavor}}/ --with-freetype-include=${HOME}/sysroot-${{ 
>> matrix.debian-arch }}/usr/include/freetype2/ 
>> --x-libraries=${HOME}/sysroot-${{ matrix.debian-arch }}/usr/lib/${{ 
>> matrix.gnu-arch }}-linux-gnu${{ matrix.gnu-flavor}}/" >> $GITHUB_ENV
> 
> No chance to break this line by option? It is kinda hard to read and thus 
> verify. It is still okay if we cannot.

Sure, I agree, perhaps just putting that as a separate step would increase 
readability.

-

PR: https://git.openjdk.java.net/jdk/pull/1350

Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]

[Robin Westberg]

On Fri, 20 Nov 2020 17:35:12 GMT, Aleksey Shipilev  wrote:

>> Looks good! A few minor nits, if you want to address them.
>
> Ah wait, I now see "Linux additional" is the column name in testing table, 
> because it is the name of the job! Eh... It was nicer to have columns per 
> arch. Does it make sense to split the "Linux x64 (other)" and "Linux Foreign" 
> then? It might also simplify the coding as we would not have to check for 
> `matrix.debian_arch`.
> 
> EDIT: Or maybe it is fine to have "Linux additional", as it stand now. Really 
> undecided here :)

I personally think I prefer to just use a single column for the additional 
cross compile builds, because the table becomes very big otherwise (and the 
rows get split up). You still see the full name of the build in case anything 
fails, so not sure it adds that much value to get an additional box for each 
cross-compiled architecture, I guess in 99% of cases they will just pass and 
then you don't really care exactly which platforms you didn't break.. But I'm 
no UX expert. :)

-

PR: https://git.openjdk.java.net/jdk/pull/1350

Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v3]

[Robin Westberg]

> Currently Linux x64 testing in GitHub Actions depends on a few non-relevant 
> hotspot build-only jobs (such as zero) that prevents testing from being run 
> if those build were to fail. As the tests only require the x64 release and 
> debug builds to run and provide interesting feedback, we should separate 
> these other builds into a different job.
> 
> To reduce duplicated steps, all the existing hotspot-only builds have been 
> consolidated into a single job.

Robin Westberg has updated the pull request incrementally with one additional 
commit since the last revision:

  Split long line

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1350/files
  - new: https://git.openjdk.java.net/jdk/pull/1350/files/c79e6ab5..75f1e659

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1350=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1350=01-02

  Stats: 13 lines in 1 file changed: 11 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1350.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1350/head:pull/1350

PR: https://git.openjdk.java.net/jdk/pull/1350

Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]

[Robin Westberg]

On Fri, 20 Nov 2020 14:39:54 GMT, Aleksey Shipilev  wrote:

>> Robin Westberg has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Improve naming, fix style issues
>
> .github/workflows/submit.yml line 380:
> 
>> 378: continue-on-error: true
>> 379: 
>> 380:   linux_x64_hotspot_only:
> 
> This is a weird name for a job that also cross-compiles non-x64 versions. 
> Maybe `linux_aux_builds` or some such?

Yeah this can probably be improved, how about the updated version?

> .github/workflows/submit.yml line 469:
> 
>> 467:   name: transient_jdk-linux-x64_${{ 
>> needs.prerequisites.outputs.bundle_id }}
>> 468:   path: ~/jdk-linux-x64
>> 469: if: steps.build_restore.outcome == 'failure'
> 
> I may be blind, but I don't see any step with id `build_restore` in new 
> file...

It already existed in the step right above so I didn't have to add it.. This 
follows the same pattern as other artifact downloads (I've seen them fail a 
fair amount of time in the past).

> .github/workflows/submit.yml line 506:
> 
>> 504: run: >
>> 505:   sudo qemu-debootstrap
>> 506:   --arch=${{ matrix.debian-arch}}
> 
> Style: `${{ matrix.debian-arch }}` (space before closing braces) -- also in 
> other places.

Ah right, good catch, fixed these.

> .github/workflows/submit.yml line 532:
> 
>> 530: run: >
>> 531:   bash configure
>> 532:   --with-conf-name=linux-x64-hotspot
> 
> That does not look a proper config name for cross-compiled jobs?

No you are right, I didn't really care since it's not visible anywhere, but can 
certainly update it a bit.

-

PR: https://git.openjdk.java.net/jdk/pull/1350

Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]

[Robin Westberg]

> Currently Linux x64 testing in GitHub Actions depends on a few non-relevant 
> hotspot build-only jobs (such as zero) that prevents testing from being run 
> if those build were to fail. As the tests only require the x64 release and 
> debug builds to run and provide interesting feedback, we should separate 
> these other builds into a different job.
> 
> To reduce duplicated steps, all the existing hotspot-only builds have been 
> consolidated into a single job.

Robin Westberg has updated the pull request incrementally with one additional 
commit since the last revision:

  Improve naming, fix style issues

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1350/files
  - new: https://git.openjdk.java.net/jdk/pull/1350/files/9cf0c80d..c79e6ab5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1350=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1350=00-01

  Stats: 21 lines in 1 file changed: 0 ins; 0 del; 21 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1350.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1350/head:pull/1350

PR: https://git.openjdk.java.net/jdk/pull/1350

RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing

[Robin Westberg]

Currently Linux x64 testing in GitHub Actions depends on a few non-relevant 
hotspot build-only jobs (such as zero) that prevents testing from being run if 
those build were to fail. As the tests only require the x64 release and debug 
builds to run and provide interesting feedback, we should separate these other 
builds into a different job.

To reduce duplicated steps, all the existing hotspot-only builds have been 
consolidated into a single job.

-

Commit messages:
 - Separate hotspot-only builds into a separate job

Changes: https://git.openjdk.java.net/jdk/pull/1350/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1350=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256747
  Stats: 517 lines in 1 file changed: 32 ins; 438 del; 47 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1350.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1350/head:pull/1350

PR: https://git.openjdk.java.net/jdk/pull/1350

Integrated: 8256393: Github Actions build on Linux should define OS and GCC versions

[Robin Westberg]

On Mon, 16 Nov 2020 12:51:25 GMT, Robin Westberg  wrote:

> We should be more explicit about OS and compiler versions used in the GitHub 
> Actions builds, to avoid problems caused by unexpected changes to the 
> defaults. This patch changes the OS and GCC versions used from ubuntu-latest 
> (currently 18.04, but will change to 20.04 sometime soon) / default 
> (currently 9.3.0) to 20.04 / 10.2.0.

This pull request has now been integrated.

Changeset: c45ab1aa
Author:    Robin Westberg 
URL:   https://git.openjdk.java.net/jdk/commit/c45ab1aa
Stats: 16 lines in 1 file changed: 3 ins; 0 del; 13 mod

8256393: Github Actions build on Linux should define OS and GCC versions

Reviewed-by: shade, erikj, ihse

-

PR: https://git.openjdk.java.net/jdk/pull/1225

Re: RFR: 8256393: Github Actions build on Linux should define OS and GCC versions [v3]

[Robin Westberg]

> We should be more explicit about OS and compiler versions used in the GitHub 
> Actions builds, to avoid problems caused by unexpected changes to the 
> defaults. This patch changes the OS and GCC versions used from ubuntu-latest 
> (currently 18.04, but will change to 20.04 sometime soon) / default 
> (currently 9.3.0) to 20.04 / 10.2.0.

Robin Westberg has updated the pull request incrementally with one additional 
commit since the last revision:

  Use proper package version

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1225/files
  - new: https://git.openjdk.java.net/jdk/pull/1225/files/3dcb26d6..12b1ffe7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1225=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1225=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1225.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1225/head:pull/1225

PR: https://git.openjdk.java.net/jdk/pull/1225

Re: RFR: 8256393: Github Actions build on Linux should define OS and GCC versions [v2]

[Robin Westberg]

On Mon, 16 Nov 2020 15:12:25 GMT, Magnus Ihse Bursie  wrote:

>> .github/workflows/submit.yml line 190:
>> 
>>> 188: run: |
>>> 189:   sudo apt-get install libxrandr-dev libxtst-dev libcups2-dev 
>>> libasound2-dev
>>> 190:   sudo update-alternatives --install /usr/bin/gcc gcc 
>>> /usr/bin/gcc-10 100 --slave /usr/bin/g++ g++ /usr/bin/g++-10
>> 
>> Maybe we should use apt-get functionality to install a specific version of 
>> packages? I'm not sure how relevant it is for the X and alsa libraries since 
>> they change very seldom, but perhaps for gcc, to get a specific point 
>> release of the compiler.
>
> Something along the lines of `sudo apt-get install 
> gcc-10=10.2.0-5ubuntu1~20`, which I believe should match quite well the 
> version used internally in the Oracle CI builds.

That sounds reasonable, I don't know how often these change in Ubuntu LTS, but 
can't hurt to be explicit here as well.

-

PR: https://git.openjdk.java.net/jdk/pull/1225

Re: RFR: 8256393: Github Actions build on Linux should define OS and GCC versions [v2]

[Robin Westberg]

> We should be more explicit about OS and compiler versions used in the GitHub 
> Actions builds, to avoid problems caused by unexpected changes to the 
> defaults. This patch changes the OS and GCC versions used from ubuntu-latest 
> (currently 18.04, but will change to 20.04 sometime soon) / default 
> (currently 9.3.0) to 20.04 / 10.2.0.

Robin Westberg has updated the pull request incrementally with one additional 
commit since the last revision:

  Select gcc/g++ package versions explicitly

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1225/files
  - new: https://git.openjdk.java.net/jdk/pull/1225/files/87ab0729..3dcb26d6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1225=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1225=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1225.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1225/head:pull/1225

PR: https://git.openjdk.java.net/jdk/pull/1225

Re: RFR: 8256393: Github Actions build on Linux should define OS and GCC versions

[Robin Westberg]

On Mon, 16 Nov 2020 13:51:29 GMT, Erik Joelsson  wrote:

>> We should be more explicit about OS and compiler versions used in the GitHub 
>> Actions builds, to avoid problems caused by unexpected changes to the 
>> defaults. This patch changes the OS and GCC versions used from ubuntu-latest 
>> (currently 18.04, but will change to 20.04 sometime soon) / default 
>> (currently 9.3.0) to 20.04 / 10.2.0.
>
> Marked as reviewed by erikj (Reviewer).

> Why does jtreg need OpenJDK-8 for building?

Fair enough, the 1.8 requirement comes from using build-all.sh which checks 
this explicitly. But I'd rather leave changing how we build jtreg for a 
separate change..

-

PR: https://git.openjdk.java.net/jdk/pull/1225

Re: RFR: 8256393: Github Actions build on Linux should define OS and GCC versions

[Robin Westberg]

On Mon, 16 Nov 2020 13:13:52 GMT, Aleksey Shipilev  wrote:

>> We should be more explicit about OS and compiler versions used in the GitHub 
>> Actions builds, to avoid problems caused by unexpected changes to the 
>> defaults. This patch changes the OS and GCC versions used from ubuntu-latest 
>> (currently 18.04, but will change to 20.04 sometime soon) / default 
>> (currently 9.3.0) to 20.04 / 10.2.0.
>
> Hold on a sec. `ubuntu-latest` is `ubuntu-18.04`, as per [GH 
> manual](https://docs.github.com/en/free-pro-team@latest/actions/reference/specifications-for-github-hosted-runners).
>  So this effectively upgrades the whole thing to Ubuntu 20.04, and upgrades 
> GCC then? I think we better stick to current defaults, i.e. `ubuntu-18.04` 
> and its GCC. 
> 
> In JDK-8256277, we did not upgrade MacOS target either...

Right, currently ubuntu-latest means 18.04, but that is only true for another 
two weeks (see https://github.com/actions/virtual-environments/issues/1816 - 
originally it was also planned for next week). So I think we should go straight 
for the upcoming latest.

> .github/workflows/submit.yml line 91:
> 
>> 89: 
>> 90:   - name: Build jtreg
>> 91: run: sh make/build-all.sh ${JAVA_HOME_8_X64}
> 
> What is this change?

On ubuntu-20.04 the default Java installation is now set to 11, but jtreg still 
requires Java 8 for building.

-

PR: https://git.openjdk.java.net/jdk/pull/1225

RFR: 8256393: Github Actions build on Linux should define OS and GCC versions

[Robin Westberg]

We should be more explicit about OS and compiler versions used in the GitHub 
Actions builds, to avoid problems caused by unexpected changes to the defaults. 
This patch changes the OS and GCC versions used from ubuntu-latest (currently 
18.04, but will change to 20.04 sometime soon) / default (currently 9.3.0) to 
20.04 / 10.2.0.

-

Commit messages:
 - Select Ubuntu 20.04 and GCC 10

Changes: https://git.openjdk.java.net/jdk/pull/1225/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1225=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256393
  Stats: 16 lines in 1 file changed: 3 ins; 0 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1225.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1225/head:pull/1225

PR: https://git.openjdk.java.net/jdk/pull/1225

Integrated: 8256354: Github Action build on Windows should define OS and MSVC versions

[Robin Westberg]

On Fri, 13 Nov 2020 17:26:28 GMT, Robin Westberg  wrote:

> We should be more explicit about OS and compiler versions used in the GitHub 
> Actions builds, to avoid problems caused by unexpected changes to the 
> defaults. This patch changes the OS and MSVC versions used from latest 
> (currently 2019) / default (currently 14.28) to 2019 / 14.27.

This pull request has now been integrated.

Changeset: 1103e337
Author:Robin Westberg 
URL:   https://git.openjdk.java.net/jdk/commit/1103e337
Stats: 9 lines in 1 file changed: 7 ins; 0 del; 2 mod

8256354: Github Action build on Windows should define OS and MSVC versions

Reviewed-by: erikj, shade

-

PR: https://git.openjdk.java.net/jdk/pull/1207

Integrated: 8256277: Github Action build on macOS should define OS and Xcode versions

[Robin Westberg]

On Fri, 13 Nov 2020 11:48:31 GMT, Robin Westberg  wrote:

> We should be more explicit about OS and compiler versions used in the GitHub 
> Actions builds, to avoid problems caused by unexpected changes to the 
> defaults. This patch changes the OS and Xcode versions used from latest 
> (currently 10.15) / default (currently 12.0) to 10.15 / 11.3.1.

This pull request has now been integrated.

Changeset: 588caab0
Author:Robin Westberg 
URL:   https://git.openjdk.java.net/jdk/commit/588caab0
Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod

8256277: Github Action build on macOS should define OS and Xcode versions

Reviewed-by: shade, ehelin, erikj

-

PR: https://git.openjdk.java.net/jdk/pull/1201

RFR: 8256354: Github Action build on Windows should define OS and MSVC versions

[Robin Westberg]

We should be more explicit about OS and compiler versions used in the GitHub 
Actions builds, to avoid problems caused by unexpected changes to the defaults. 
This patch changes the OS and MSVC versions used from latest (currently 2019) / 
default (currently 14.28) to 2019 / 14.27.

-

Commit messages:
 - Use Windows 2019 and MSVC 14.27

Changes: https://git.openjdk.java.net/jdk/pull/1207/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1207=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256354
  Stats: 9 lines in 1 file changed: 7 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1207.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1207/head:pull/1207

PR: https://git.openjdk.java.net/jdk/pull/1207

Re: RFR: 8256277: Github Action build on macOS should define OS and Xcode versions

[Robin Westberg]

On Fri, 13 Nov 2020 11:58:19 GMT, Aleksey Shipilev  wrote:

>> We should be more explicit about OS and compiler versions used in the GitHub 
>> Actions builds, to avoid problems caused by unexpected changes to the 
>> defaults. This patch changes the OS and Xcode versions used from latest 
>> (currently 10.15) / default (currently 12.0) to 10.15 / 11.3.1.
>
> Looks fine to me. I was wondering if we should do the same for 
> `ubuntu-latest` and `windows-latest`.

Thanks for reviewing! Yeah, planning to do something similar for the other 
platforms as well!

-

PR: https://git.openjdk.java.net/jdk/pull/1201

RFR: 8256277: Github Action build on macOS should define OS and Xcode versions

[Robin Westberg]

We should be more explicit about OS and compiler versions used in the GitHub 
Actions builds, to avoid problems caused by unexpected changes to the defaults. 
This patch changes the OS and Xcode versions used from latest (currently 10.15) 
/ default (currently 12.0) to 10.15 / 11.3.1.

-

Commit messages:
 - Select explicit versions of macOS and Xcode

Changes: https://git.openjdk.java.net/jdk/pull/1201/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1201=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256277
  Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1201/head:pull/1201

PR: https://git.openjdk.java.net/jdk/pull/1201

Re: RFR: 8256127: Add cross-compiled foreign architectures builds to submit workflow [v6]

[Robin Westberg]

On Fri, 13 Nov 2020 10:54:01 GMT, Aleksey Shipilev  wrote:

>> Looks good! I did a similar attempt at cross building a while ago, but never 
>> got around to finishing it, so it's nice to see it materializing! I do have 
>> a general comment on reducing the amount of duplicated content though. Since 
>> all these cross-build platforms share the same prerequisites, they can be 
>> expressed as matrix builds. Here's what I did: 
>> https://github.com/rwestberg/jdk/blob/947c934621c3013c055152356615e0120382cedf/.github/workflows/submit.yml#L102
>> 
>> You'd have to adjust the details obviously, but I think it could help with 
>> future maintainability.
>> 
>> Another minor comment is that it may be faster to use 
>> http://debian-archive.trafficmanager.net/debian/ instead of 
>> http://httpredir.debian.org/debian/ (the former is Azure-specific but I 
>> don't think it's part of the list that the latter uses). But since it will 
>> be cached after first use it probably doesn't matter much.
>
>> Looks good! I did a similar attempt at cross building a while ago, but never 
>> got around to finishing it, so it's nice to see it materializing! I do have 
>> a general comment on reducing the amount of duplicated content though. Since 
>> all these cross-build platforms share the same prerequisites, they can be 
>> expressed as matrix builds. Here's what I did: 
>> https://github.com/rwestberg/jdk/blob/947c934621c3013c055152356615e0120382cedf/.github/workflows/submit.yml#L102
> 
> Right. AFAIU your code, it bootstraps the chroot and builds x86_64 build JDK 
> for every config and every run, something this PR is able to avoid. We'd need 
> to figure that out. I think that is pretty doable, but it would require a few 
> days worth of pipeline testing to work out the kinks. So, would you mind we 
> do that in the follow-ups?

Yeah, it would certainly need a bit of adaptation to capture the differences 
properly, so perfectly fine to look into later!

-

PR: https://git.openjdk.java.net/jdk/pull/1147

Re: RFR: 8256127: Add cross-compiled foreign architectures builds to submit workflow [v6]

[Robin Westberg]

On Thu, 12 Nov 2020 06:50:08 GMT, Aleksey Shipilev  wrote:

>> It is 
>> [possible](https://github.com/openjdk/jdk/blob/master/doc/building.md#creating-and-using-sysroots-with-qemu-deboostrap)
>>  to efficiently cross-compile to foreign architectures on current GH actions 
>> that are driven by Ubuntu. I have been using this method for years to 
>> produce binaries at [builds.shipilev.net](https://builds.shipilev.net). 
>> 
>> These cross-compilation targets frequently find arch-specific build bugs. 
>> Hotspot is rather well insulated from the fundamental problems in 
>> arch-support, so the overwhelming majority of those arch-specific bugs are 
>> simple omissions of `#include`-s, inconsistent `#ifdef`-ing, missed method 
>> renames / signature changes, or incorrect removals of methods that are used 
>> by arch-specific code. Those are straightforward to fix, if the contributor 
>> knows about them. My experience says the common attitude to these fixes is: 
>> "Oh, I would have fixed it in the original change if I knew about this." 
>> 
>> This improvement adds several foreign architectures to GH actions workflow 
>> to provide the automatic safety net to give early warning for these cases. 
>> In fact, many Hotspot contributors already build AArch64, ARM, PPC64 targets 
>> as pre-integration sanity checks, and this does the important checks 
>> automatically for them.
>> 
>> To optimize workflow costs, the change does the following:
>>   - The build is only done for hotspot-debug-no-pch, as the most frequent 
>> place where arch-specific build bugs show up. 
>>   - There are no tests, and in fact the arch-specific binary does not even 
>> run. The build is purely about the build.
>>   - Foreign architectures reuse the Linux x86_64 release build as build JDK. 
>> This avoids building "host" build JDK as it would otherwise happen with 
>> cross-compiling.
>>   - The created sysroot is cached and keyed on `submit.yml` hash. So it 
>> would regenerate only if the workflow itself changes.  This saves about 10 
>> minutes per arch.
>> 
>> Space-wise, sysroots in GH cache take about 580M uncompressed, and 270M 
>> zstd-compressed bundle. In the end, this adds 4x270 = 1080M into local cache.
>> 
>> Time-wise, ball-parking the current workflow budget [looks like 
>> this](https://github.com/shipilev/jdk/runs/1389151071):
>>  - Linux x86_64:
>>- builds: 120m
>>- tests: 200m
>>  - Linux x86_32:
>>- builds: 75m
>>- tests: 235m
>>  - Windows x86_64
>>- builds: 120m
>>- tests: 290m
>>  - MacOS x86_64
>>- builds: 75m
>>- tests: 165m
>>  - Linux aarch64:
>>- builds: 25m (+10m to create uncached sysroot)
>>  - Linux arm:
>>- builds: 20m (+10m to create uncached sysroot)
>>  - Linux ppc64le:
>>- builds: 20m (+15m to create uncached sysroot)
>>  - Linux s390x:
>>- builds: 20m (+10m to create uncached sysroot)
>> 
>> In other words, new workflow takes about 715 Linux-host-minutes, 410 
>> Windows-host-minutes, 240 Mac-host-minutes. Out of which new jobs take about 
>> 85 Linux-host-minutes. So the cost of new jobs is roughly:
>>   - 11.9% of Linux-host-minutes
>>   - 6.2% of all-host-minutes
>>   - 2.2% of runner-minutes, if you [weigh in the proportional cost of 
>> non-Linux 
>> runners](https://docs.github.com/en/free-pro-team@latest/github/setting-up-and-managing-billing-and-payments-on-github/about-billing-for-github-actions)
>> 
>> In other words, the additional runner costs are pale in comparison with what 
>> is already done in build and test jobs.
>
> Aleksey Shipilev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Build only hotspot reduced build

Looks good! I did a similar attempt at cross building a while ago, but never 
got around to finishing it, so it's nice to see it materializing! I do have a 
general comment on reducing the amount of duplicated content though. Since all 
these cross-build platforms share the same prerequisites, they can be expressed 
as matrix builds. Here's what I did: 
https://github.com/rwestberg/jdk/blob/947c934621c3013c055152356615e0120382cedf/.github/workflows/submit.yml#L102

You'd have to adjust the details obviously, but I think it could help with 
future maintainability.

Another minor comment is that it may be faster to use 
http://debian-archive.trafficmanager.net/debian/ instead of 
http://httpredir.debian.org/debian/ (the former is Azure-specific but I don't 
think it's part of the list that the latter uses). But since it will be cached 
after first use it probably doesn't matter much.

-

Marked as reviewed by rwestberg (Committer).

PR: https://git.openjdk.java.net/jdk/pull/1147

Re: RFR: 8255352: Archive important test outputs in submit workflow

[Robin Westberg]

On Fri, 23 Oct 2020 17:18:57 GMT, Aleksey Shipilev  wrote:

> Currently, we are only archiving `build/*/test-results`. But hs_errs, 
> replays, and test outputs are actually in `build/*/test-support`! Which means 
> once any test fails, we only have the summary of the run, not the bits that 
> would actually help to diagnose the problem. 
> 
> Archiving the entire test-support seems too much, it yields 50K artifacts 
> available as 1 GB zip archive in GH GUI. On top of that `upload-artifact` 
> does the upload per-file, which takes tens of minutes on 50K files (and I 
> suspect many API calls).
> 
> But we can pick up important test outputs selectively and then also compress 
> them. See sample artifact here:
>  https://github.com/shipilev/jdk/runs/1301540541
> 
> It packages the final ZIP like this:
> 
> $ unzip -t test-results_.zip
>  Archive:  test-results_.zip
> testing: artifact/OK
> testing: artifact/linux-x64-debug_testresults_hs_tier1_common.zip   OK
> testing: artifact/linux-x64-debug_testsupport_hs_tier1_common.zip   OK
> ...

Looks good! I also tried including the entire test-support folder at some 
point, but as you noticed it takes quite a bit of time and space. So this 
approach seems much better.

Minor comment: The cygwin\bin folder is included twice in PATH on Windows to 
work around a bug in JTReg (the regex used to detect cygwin does not match the 
very first entry in the PATH list. Perhaps I should file a bug for that..) but 
it's not really needed if you are executing something else.

.github/workflows/submit.yml line 768:

> 766: working-directory: build/run-test-prebuilt/test-results/
> 767: run: >
> 768:   $env:Path = 
> "$HOME\cygwin\cygwin64\bin;$HOME\cygwin\cygwin64\bin;$env:Path" ;

Suggestion:

  $env:Path = "$HOME\cygwin\cygwin64\bin;$env:Path" ;

.github/workflows/submit.yml line 778:

> 776: working-directory: build/run-test-prebuilt/test-support/
> 777: run: >
> 778:   $env:Path = 
> "$HOME\cygwin\cygwin64\bin;$HOME\cygwin\cygwin64\bin;$env:Path" ;

Suggestion:

  $env:Path = "$HOME\cygwin\cygwin64\bin;$env:Path" ;

-

Marked as reviewed by rwestberg (Committer).

PR: https://git.openjdk.java.net/jdk/pull/842

Re: RFR: 8255373: Submit workflow artifact name is always "test-results_.zip"

[Robin Westberg]

On Sat, 24 Oct 2020 08:35:31 GMT, Aleksey Shipilev  wrote:

> The output for the `prerequisites` step is `bundle_id: ${{ 
> steps.check_bundle_id.outputs.bundle_id }}`, but there is no 
> `check_bundle_id` step name to properly reference. Then `artifacts` should 
> actually need `prerequisites` to access 
> `needs.prerequisites.outputs.bundle_id`.
> 
> See the final artifact name in the workflow:
>   https://github.com/shipilev/jdk/actions/runs/325845086

Good catch, thanks for fixing!

-

Marked as reviewed by rwestberg (Committer).

PR: https://git.openjdk.java.net/jdk/pull/849

Re: RFR: 8254282: Add Linux x86_32 builds to submit workflow

[Robin Westberg]

On Mon, 12 Oct 2020 12:46:57 GMT, Aleksey Shipilev  wrote:

> Building x86_32 usually exposes 32/64 cleanliness problems early. Since 
> 32-bit builds break often, it might make sense
> to add them to submit workflow. The x86_32 might not be widely deployed by 
> itself, but 32/64 bit cleanliness detects
> bugs that manifest on ARM32 early.

Looks good!

-

Marked as reviewed by rwestberg (Committer).

PR: https://git.openjdk.java.net/jdk/pull/608

Re: RFR: 8254173: Add Zero, Minimal hotspot targets to submit workflow [v3]

[Robin Westberg]

On Thu, 8 Oct 2020 07:20:39 GMT, Aleksey Shipilev  wrote:

>> Zero VM and Minimal VM builds are routinely discovering the problems with 
>> internal Hotspot dependencies. Mostly because
>> they turn off the whole lot of VM features, and every path that is not 
>> guarded by a feature #ifdef or build file list
>> fails.   It would be good to add Zero and Minimal targets to submit workflow.
>> 
>> It seems to add two ~9 minute stages, compared to ~17 minute stage for 
>> building no-pch hotspot, and ~33 minutes for
>> full JDK.
>> Attention @rwestberg.
>> 
>> Testing:
>>   - [x] GH workflow still works; see [latest run 
>> ](https://github.com/shipilev/jdk/actions/runs/293805791)
>
> Aleksey Shipilev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Simplify step names

Looks good, thanks for fixing!

-

Marked as reviewed by rwestberg (Committer).

PR: https://git.openjdk.java.net/jdk/pull/546

Re: RFR: 8254175: Build no-pch configuration in debug mode for submit checks [v2]

[Robin Westberg]

On Thu, 8 Oct 2020 07:19:21 GMT, Aleksey Shipilev  wrote:

>> no-pch configuration is supposed to expose missing include dependencies. But 
>> currently it runs with default (release)
>> bits, which misses symbols hidden in debug code. We should consider building 
>> it in debug mode.
>> Attention @rwestberg.
>> 
>> Testing:
>>   - [x] GH workflow still works, see the builds in the [latest 
>> run](https://github.com/shipilev/jdk/actions/runs/293802758)
>
> Aleksey Shipilev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Drop "debug" from the names

Looks good to me! Just a similar comment as in #546, the name is a bit too long 
now, so perhaps just drop the "debug"
from the description?

Looks good!

.github/workflows/submit.yml line 108:

> 106:   - build release
> 107:   - build debug
> 108:   - build debug hotspot no-pch

Suggestion:

  - build hotspot no-pch

.github/workflows/submit.yml line 113:

> 111: flags: --enable-debug
> 112: artifact: -debug
> 113:   - flavor: build debug hotspot no-pch

Suggestion:

  - flavor: build hotspot no-pch

-

Marked as reviewed by rwestberg (Committer).

PR: https://git.openjdk.java.net/jdk/pull/547

Re: RFR: 8254173: Add Zero, Minimal hotspot targets to submit workflow [v2]

[Robin Westberg]

On Thu, 8 Oct 2020 07:12:12 GMT, Aleksey Shipilev  wrote:

>> That makes sense. We should change the order in #547 then first? So the 
>> final thing would be:
>> 
>> - build release
>> - build debug
>> - build hotspot no-pch debug
>> - build hotspot zero no-pch debug
>> - build hotspot minimal no-pch debug
>
> ...or we drop `debug` to altogether:
> 
> - build release
> - build debug
> - build hotspot no-pch
> - build hotspot zero no-pch
> - build hotspot minimal no-pch

Yeah I had the same comment there. :) Perhaps just drop "debug" qualifier as 
they all have it. And perhaps drop the
no-pch from the two new ones, it's IMO not that important to display it in the 
description (unless we were to add for
example zero without no-pch) - the full config is fairly easy to figure out 
from the yml or the test log anyway.

-

PR: https://git.openjdk.java.net/jdk/pull/546

Re: RFR: 8254173: Add Zero, Minimal hotspot targets to submit workflow [v2]

[Robin Westberg]

On Wed, 7 Oct 2020 16:28:17 GMT, Aleksey Shipilev  wrote:

>> Zero VM and Minimal VM builds are routinely discovering the problems with 
>> internal Hotspot dependencies. Mostly because
>> they turn off the whole lot of VM features, and every path that is not 
>> guarded by a feature #ifdef or build file list
>> fails.   It would be good to add Zero and Minimal targets to submit workflow.
>> 
>> It seems to add two ~9 minute stages, compared to ~17 minute stage for 
>> building no-pch hotspot, and ~33 minutes for
>> full JDK.
>> Attention @rwestberg.
>> 
>> Testing:
>>   - [x] GH workflow still works; see [latest run 
>> ](https://github.com/shipilev/jdk/actions/runs/293805791)
>
> Aleksey Shipilev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Build with debug and no-pch for zero and minimal

Looks good to me! Adding a few additional shorter build shouldn't increase the 
total runtime, as the test execution
waits for all builds to be done, so we are limited by the full builds anyway.

One minor comment, perhaps rearrange the description a bit of these new flavors 
with the differentiating parts first.
GitHub cuts them short in the overview list (see the list on the left at
https://github.com/shipilev/jdk/actions/runs/293805791 for an example) so you 
can't currently tell which one is which
in the overview without clicking on them.

.github/workflows/submit.yml line 109:

> 107:   - build debug
> 108:   - build hotspot no-pch
> 109:   - build debug hotspot no-pch zero

Suggestion:

  - build hotspot zero no-pch debug

-

Marked as reviewed by rwestberg (Committer).

PR: https://git.openjdk.java.net/jdk/pull/546

Integrated: 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command

[Robin Westberg]

On Tue, 6 Oct 2020 06:39:55 GMT, Robin Westberg  wrote:

> The `set-env` command was recently deprecated in favor of a different method 
> of setting environment variables, due to a
> security vulnerability [1]. The vulnerability does not affect our usage of 
> GitHub Actions, but we should nevertheless
> update to avoid the associated deprecation warnings.   [1]
> https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/

This pull request has now been integrated.

Changeset: d2b1dc6d
Author:Robin Westberg 
URL:   https://git.openjdk.java.net/jdk/commit/d2b1dc6d
Stats: 53 lines in 1 file changed: 1 ins; 21 del; 31 mod

8254054: Pre-submit testing using GitHub Actions should not use the deprecated 
set-env command

Reviewed-by: ehelin, erikj

-

PR: https://git.openjdk.java.net/jdk/pull/518

RFR: 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command

[Robin Westberg]

The `set-env` command was recently deprecated in favor of a different method of 
setting environment variables, due to a
security vulnerability [1]. The vulnerability does not affect our usage of 
GitHub Actions, but we should nevertheless
update to avoid the associated deprecation warnings.

[1] 
https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/

-

Commit messages:
 - Remove usage of deprecated ::set-env

Changes: https://git.openjdk.java.net/jdk/pull/518/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=518=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8254054
  Stats: 53 lines in 1 file changed: 1 ins; 21 del; 31 mod
  Patch: https://git.openjdk.java.net/jdk/pull/518.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/518/head:pull/518

PR: https://git.openjdk.java.net/jdk/pull/518

Integrated: 8253865: Pre-submit testing using GitHub Actions does not detect failures reliably

[Robin Westberg]

On Wed, 30 Sep 2020 16:58:25 GMT, Robin Westberg  wrote:

> The pre-submit test definitions utilizing GitHub Actions can sometimes report 
> success even when there are failing
> tests. The failure detection depended on make returning a non-zero exit code 
> on failures, which doesn't seem to work.
> To properly determine test outcome we should check the "test-summary.txt" 
> result file for the string "TEST SUCCESS". If
> it isn't found, the tests must have failed. This change also includes a 
> reliability improvement.
> Here's an example of a run with the updated test definitions: 
> https://github.com/rwestberg/jdk/actions/runs/280529475
> 
> Note! There is a failure now properly detected in langtools/tier1 on Windows
> (tools/javac/launcher/SourceLauncherTest.java) which is tracked by 
> https://bugs.openjdk.java.net/browse/JDK-8249095 -
> could be of interest to either fix or ProblemList that one before integrating 
> this change.

This pull request has now been integrated.

Changeset: 7dcdc1fb
Author:Robin Westberg 
URL:   https://git.openjdk.java.net/jdk/commit/7dcdc1fb
Stats: 108 lines in 1 file changed: 108 ins; 0 del; 0 mod

8253865: Pre-submit testing using GitHub Actions does not detect failures 
reliably
8253867: Pre-submit testing using GitHub Actions can fail to download 
intermediate artifacts

Reviewed-by: erikj

-

PR: https://git.openjdk.java.net/jdk/pull/437

RFR: 8253865: Pre-submit testing using GitHub Actions does not detect failures reliably

[Robin Westberg]

The pre-submit test definitions utilizing GitHub Actions can sometimes report 
success even when there are failing
tests. The failure detection depended on make returning a non-zero exit code on 
failures, which doesn't seem to work.

To properly determine test outcome we should check the "test-summary.txt" 
result file for the string "TEST SUCCESS". If
it isn't found, the tests must have failed. This change also includes a 
reliability improvement.

Here's an example of a run with the updated test definitions: 
https://github.com/rwestberg/jdk/actions/runs/280529475

Note! There is a failure now properly detected in langtools/tier1 on Windows
(tools/javac/launcher/SourceLauncherTest.java) which is tracked by 
https://bugs.openjdk.java.net/browse/JDK-8249095 -
could be of interest to either fix or ProblemList that one before integrating 
this change.

-

Commit messages:
 - Retry artifact downloading on failure
 - Check results and show comprehensive list of failing tests (if any)

Changes: https://git.openjdk.java.net/jdk/pull/437/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=437=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8253865
  Stats: 108 lines in 1 file changed: 108 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/437.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/437/head:pull/437

PR: https://git.openjdk.java.net/jdk/pull/437

Integrated: 8253424: Add support for running pre-submit testing using GitHub Actions

[Robin Westberg]

On Mon, 21 Sep 2020 13:32:09 GMT, Robin Westberg  wrote:

> A few days ago I posted an initial version of the necessary configuration 
> required to run pre-submit build and tests
> for JDK main-line contributions using GitHub Actions [2] and the free tier 
> [3] available to everyone working with open
> source repositories. I've incorporated the feedback into an updated version 
> that I believe is ready to be integrated.
> If this is integrated into the `master` branch, future branches created and 
> updated in personal forks will build and
> run the basic tier 1 tests as described in this configuration, on Linux, 
> Windows and macOS (all on x64). It's of course
> possible for any contributor to opt out fully or partially of these automatic 
> runs in a few different ways.  To opt out
> completely, a contributor can simply disable GitHub Actions on their personal 
> fork, and no further jobs will be
> executed. Another option is to add a repository secret [4] with the name 
> `JDK_SUBMIT_FILTER` set to any value. If this
> is set, only branches prefixed with `submit/` will be subject to automatic 
> build and test. This can also be further
> refined by adding a repository secret named `JDK_SUBMIT_PLATFORMS` with a 
> value such as `Linux x64, Windows x64` to
> limit automatic build and test to these two platforms. It will still be 
> possible to run the tests on any branch and/or
> platform by manually triggering the workflow.  To see what this looks like in 
> practice, an example run can be found
> here: https://github.com/rwestberg/jdk/actions/runs/265131985 (note that 
> there is currently a failing test on Windows
> which is tracked by JDK-8249095, which should probably be resolved before 
> this change is integrated).  Best regards,
> Robin  [1] 
> https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html [2]
> https://github.com/features/actions [3]
> https://docs.github.com/en/actions/getting-started-with-github-actions/about-github-actions#usage-limits
>  [4]
> https://docs.github.com/en/actions/reference/encrypted-secrets

This pull request has now been integrated.

Changeset: 840aa2b7
Author:Robin Westberg 
URL:   https://git.openjdk.java.net/jdk/commit/840aa2b7
Stats: 904 lines in 2 files changed: 904 ins; 0 del; 0 mod

8253424: Add support for running pre-submit testing using GitHub Actions

Reviewed-by: ehelin, erikj

-

PR: https://git.openjdk.java.net/jdk/pull/284

Re: RFR: 8253424: Add support for running pre-submit testing using GitHub Actions [v7]

[Robin Westberg]

> A few days ago I posted an initial version of the necessary configuration 
> required to run pre-submit build and tests
> for JDK main-line contributions using GitHub Actions [2] and the free tier 
> [3] available to everyone working with open
> source repositories. I've incorporated the feedback into an updated version 
> that I believe is ready to be integrated.
> If this is integrated into the `master` branch, future branches created and 
> updated in personal forks will build and
> run the basic tier 1 tests as described in this configuration, on Linux, 
> Windows and macOS (all on x64). It's of course
> possible for any contributor to opt out fully or partially of these automatic 
> runs in a few different ways.  To opt out
> completely, a contributor can simply disable GitHub Actions on their personal 
> fork, and no further jobs will be
> executed. Another option is to add a repository secret [4] with the name 
> `JDK_SUBMIT_FILTER` set to any value. If this
> is set, only branches prefixed with `submit/` will be subject to automatic 
> build and test. This can also be further
> refined by adding a repository secret named `JDK_SUBMIT_PLATFORMS` with a 
> value such as `Linux x64, Windows x64` to
> limit automatic build and test to these two platforms. It will still be 
> possible to run the tests on any branch and/or
> platform by manually triggering the workflow.  To see what this looks like in 
> practice, an example run can be found
> here: https://github.com/rwestberg/jdk/actions/runs/265131985 (note that 
> there is currently a failing test on Windows
> which is tracked by JDK-8249095, which should probably be resolved before 
> this change is integrated).  Best regards,
> Robin  [1] 
> https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html [2]
> https://github.com/features/actions [3]
> https://docs.github.com/en/actions/getting-started-with-github-actions/about-github-actions#usage-limits
>  [4]
> https://docs.github.com/en/actions/reference/encrypted-secrets

Robin Westberg has updated the pull request incrementally with one additional 
commit since the last revision:

  Final optimization

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/284/files
  - new: https://git.openjdk.java.net/jdk/pull/284/files/22336eba..c7830155

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=284=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=284=05-06

  Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/284.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/284/head:pull/284

PR: https://git.openjdk.java.net/jdk/pull/284

Re: RFR: 8253424: Add support for running pre-submit testing using GitHub Actions [v6]

[Robin Westberg]

> A few days ago I posted an initial version of the necessary configuration 
> required to run pre-submit build and tests
> for JDK main-line contributions using GitHub Actions [2] and the free tier 
> [3] available to everyone working with open
> source repositories. I've incorporated the feedback into an updated version 
> that I believe is ready to be integrated.
> If this is integrated into the `master` branch, future branches created and 
> updated in personal forks will build and
> run the basic tier 1 tests as described in this configuration, on Linux, 
> Windows and macOS (all on x64). It's of course
> possible for any contributor to opt out fully or partially of these automatic 
> runs in a few different ways.  To opt out
> completely, a contributor can simply disable GitHub Actions on their personal 
> fork, and no further jobs will be
> executed. Another option is to add a repository secret [4] with the name 
> `JDK_SUBMIT_FILTER` set to any value. If this
> is set, only branches prefixed with `submit/` will be subject to automatic 
> build and test. This can also be further
> refined by adding a repository secret named `JDK_SUBMIT_PLATFORMS` with a 
> value such as `Linux x64, Windows x64` to
> limit automatic build and test to these two platforms. It will still be 
> possible to run the tests on any branch and/or
> platform by manually triggering the workflow.  To see what this looks like in 
> practice, an example run can be found
> here: https://github.com/rwestberg/jdk/actions/runs/265131985 (note that 
> there is currently a failing test on Windows
> which is tracked by JDK-8249095, which should probably be resolved before 
> this change is integrated).  Best regards,
> Robin  [1] 
> https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html [2]
> https://github.com/features/actions [3]
> https://docs.github.com/en/actions/getting-started-with-github-actions/about-github-actions#usage-limits
>  [4]
> https://docs.github.com/en/actions/reference/encrypted-secrets

Robin Westberg has updated the pull request incrementally with one additional 
commit since the last revision:

  Ensure that test logs are always collected, regardless of test job outcome

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/284/files
  - new: https://git.openjdk.java.net/jdk/pull/284/files/441a18b7..22336eba

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=284=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=284=04-05

  Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/284.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/284/head:pull/284

PR: https://git.openjdk.java.net/jdk/pull/284

Re: RFR: 8253424: Add support for running pre-submit testing using GitHub Actions

[Robin Westberg]

On Wed, 23 Sep 2020 17:55:23 GMT, Robin Westberg  wrote:

>> The version-numbers file (which is also a shared properties style file) is 
>> not using quotes for values, which is fine
>> as long as there are no spaces. I believe if you read it as a properties 
>> file, you need to strip the quotes if you have
>> them. I prefer if version-numbers and test-dependencies using the same 
>> format.  Otherwise this looks good to me!
>
> Sounds reasonable, didn't notice that discrepancy! Fixed in the latest commit.

After some feedback on the jdk-dev mailing list (thanks @jaikiran!) I took a 
second look at removing artifacts that are
only created for passing between the build and test steps. Turns out that there 
is an API for this (it's still in
preview, but seems to work fine) that can be used for this. When this API is 
finalized we can update that part, but
it's not a deal-breaker even if it should change, as the test runs will still 
complete successfully. With this API
available, it's also possible to publish a single artifact containing all test 
results, regardless of outcome.

-

PR: https://git.openjdk.java.net/jdk/pull/284

Re: RFR: 8253424: Add support for running pre-submit testing using GitHub Actions [v5]

[Robin Westberg]

> A few days ago I posted an initial version of the necessary configuration 
> required to run pre-submit build and tests
> for JDK main-line contributions using GitHub Actions [2] and the free tier 
> [3] available to everyone working with open
> source repositories. I've incorporated the feedback into an updated version 
> that I believe is ready to be integrated.
> If this is integrated into the `master` branch, future branches created and 
> updated in personal forks will build and
> run the basic tier 1 tests as described in this configuration, on Linux, 
> Windows and macOS (all on x64). It's of course
> possible for any contributor to opt out fully or partially of these automatic 
> runs in a few different ways.  To opt out
> completely, a contributor can simply disable GitHub Actions on their personal 
> fork, and no further jobs will be
> executed. Another option is to add a repository secret [4] with the name 
> `JDK_SUBMIT_FILTER` set to any value. If this
> is set, only branches prefixed with `submit/` will be subject to automatic 
> build and test. This can also be further
> refined by adding a repository secret named `JDK_SUBMIT_PLATFORMS` with a 
> value such as `Linux x64, Windows x64` to
> limit automatic build and test to these two platforms. It will still be 
> possible to run the tests on any branch and/or
> platform by manually triggering the workflow.  To see what this looks like in 
> practice, an example run can be found
> here: https://github.com/rwestberg/jdk/actions/runs/265131985 (note that 
> there is currently a failing test on Windows
> which is tracked by JDK-8249095, which should probably be resolved before 
> this change is integrated).  Best regards,
> Robin  [1] 
> https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html [2]
> https://github.com/features/actions [3]
> https://docs.github.com/en/actions/getting-started-with-github-actions/about-github-actions#usage-limits
>  [4]
> https://docs.github.com/en/actions/reference/encrypted-secrets

Robin Westberg has updated the pull request incrementally with one additional 
commit since the last revision:

  Avoid persisting transient artifacts, combine all test results into a final 
bundle

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/284/files
  - new: https://git.openjdk.java.net/jdk/pull/284/files/de5074fe..441a18b7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=284=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=284=03-04

  Stats: 92 lines in 1 file changed: 64 ins; 6 del; 22 mod
  Patch: https://git.openjdk.java.net/jdk/pull/284.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/284/head:pull/284

PR: https://git.openjdk.java.net/jdk/pull/284

Re: RFR: 8253424: Add support for running pre-submit testing using GitHub Actions [v4]

[Robin Westberg]

> A few days ago I posted an initial version of the necessary configuration 
> required to run pre-submit build and tests
> for JDK main-line contributions using GitHub Actions [2] and the free tier 
> [3] available to everyone working with open
> source repositories. I've incorporated the feedback into an updated version 
> that I believe is ready to be integrated.
> If this is integrated into the `master` branch, future branches created and 
> updated in personal forks will build and
> run the basic tier 1 tests as described in this configuration, on Linux, 
> Windows and macOS (all on x64). It's of course
> possible for any contributor to opt out fully or partially of these automatic 
> runs in a few different ways.  To opt out
> completely, a contributor can simply disable GitHub Actions on their personal 
> fork, and no further jobs will be
> executed. Another option is to add a repository secret [4] with the name 
> `JDK_SUBMIT_FILTER` set to any value. If this
> is set, only branches prefixed with `submit/` will be subject to automatic 
> build and test. This can also be further
> refined by adding a repository secret named `JDK_SUBMIT_PLATFORMS` with a 
> value such as `Linux x64, Windows x64` to
> limit automatic build and test to these two platforms. It will still be 
> possible to run the tests on any branch and/or
> platform by manually triggering the workflow.  To see what this looks like in 
> practice, an example run can be found
> here: https://github.com/rwestberg/jdk/actions/runs/265131985 (note that 
> there is currently a failing test on Windows
> which is tracked by JDK-8249095, which should probably be resolved before 
> this change is integrated).  Best regards,
> Robin  [1] 
> https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html [2]
> https://github.com/features/actions [3]
> https://docs.github.com/en/actions/getting-started-with-github-actions/about-github-actions#usage-limits
>  [4]
> https://docs.github.com/en/actions/reference/encrypted-secrets

Robin Westberg has updated the pull request incrementally with one additional 
commit since the last revision:

  Update boot JDK to 15

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/284/files
  - new: https://git.openjdk.java.net/jdk/pull/284/files/496d896c..de5074fe

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=284=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=284=02-03

  Stats: 10 lines in 1 file changed: 0 ins; 0 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/284.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/284/head:pull/284

PR: https://git.openjdk.java.net/jdk/pull/284

Re: RFR: 8248984: Bump minimum boot jdk to JDK 15

[Robin Westberg]

On Wed, 23 Sep 2020 22:47:47 GMT, Erik Joelsson  wrote:

>> JDK 15 is now GA. The minimum boot JDK version for mainline/JDK 16 should be 
>> bumped to this version.
>> 
>> Testing: tier1-5 passed with a slightly earlier version of this change. 
>> Re-running tier1 now for good luck.
>
> @rwestberg Note that you will need to update your actions patch if this goes 
> in first.

@erikj79 Thanks for the heads-up, I've updated #284 to use 15 now.

-

PR: https://git.openjdk.java.net/jdk/pull/326

Re: RFR: 8253424: Add support for running pre-submit testing using GitHub Actions

[Robin Westberg]

On Wed, 23 Sep 2020 17:24:39 GMT, Erik Joelsson  wrote:

>> I added `make/conf/test-dependencies` with version numbers specified on the 
>> format that `jib-profiles.js` would expect.
>> Actually using them from that file as well could perhaps be a separate task 
>> though. :)
>
> The version-numbers file (which is also a shared properties style file) is 
> not using quotes for values, which is fine
> as long as there are no spaces. I believe if you read it as a properties 
> file, you need to strip the quotes if you have
> them. I prefer if version-numbers and test-dependencies using the same 
> format.  Otherwise this looks good to me!

Sounds reasonable, didn't notice that discrepancy! Fixed in the latest commit.

-

PR: https://git.openjdk.java.net/jdk/pull/284

Re: RFR: 8253424: Add support for running pre-submit testing using GitHub Actions [v3]

[Robin Westberg]

> A few days ago I posted an initial version of the necessary configuration 
> required to run pre-submit build and tests
> for JDK main-line contributions using GitHub Actions [2] and the free tier 
> [3] available to everyone working with open
> source repositories. I've incorporated the feedback into an updated version 
> that I believe is ready to be integrated.
> If this is integrated into the `master` branch, future branches created and 
> updated in personal forks will build and
> run the basic tier 1 tests as described in this configuration, on Linux, 
> Windows and macOS (all on x64). It's of course
> possible for any contributor to opt out fully or partially of these automatic 
> runs in a few different ways.  To opt out
> completely, a contributor can simply disable GitHub Actions on their personal 
> fork, and no further jobs will be
> executed. Another option is to add a repository secret [4] with the name 
> `JDK_SUBMIT_FILTER` set to any value. If this
> is set, only branches prefixed with `submit/` will be subject to automatic 
> build and test. This can also be further
> refined by adding a repository secret named `JDK_SUBMIT_PLATFORMS` with a 
> value such as `Linux x64, Windows x64` to
> limit automatic build and test to these two platforms. It will still be 
> possible to run the tests on any branch and/or
> platform by manually triggering the workflow.  To see what this looks like in 
> practice, an example run can be found
> here: https://github.com/rwestberg/jdk/actions/runs/265131985 (note that 
> there is currently a failing test on Windows
> which is tracked by JDK-8249095, which should probably be resolved before 
> this change is integrated).  Best regards,
> Robin  [1] 
> https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html [2]
> https://github.com/features/actions [3]
> https://docs.github.com/en/actions/getting-started-with-github-actions/about-github-actions#usage-limits
>  [4]
> https://docs.github.com/en/actions/reference/encrypted-secrets

Robin Westberg has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove unnecessary quoting from the test-dependencies file

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/284/files
  - new: https://git.openjdk.java.net/jdk/pull/284/files/7f260ede..496d896c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=284=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=284=01-02

  Stats: 13 lines in 1 file changed: 0 ins; 0 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/284.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/284/head:pull/284

PR: https://git.openjdk.java.net/jdk/pull/284

Re: RFR: 8253424: Add support for running pre-submit testing using GitHub Actions [v2]

[Robin Westberg]

> A few days ago I posted an initial version of the necessary configuration 
> required to run pre-submit build and tests
> for JDK main-line contributions using GitHub Actions [2] and the free tier 
> [3] available to everyone working with open
> source repositories. I've incorporated the feedback into an updated version 
> that I believe is ready to be integrated.
> If this is integrated into the `master` branch, future branches created and 
> updated in personal forks will build and
> run the basic tier 1 tests as described in this configuration, on Linux, 
> Windows and macOS (all on x64). It's of course
> possible for any contributor to opt out fully or partially of these automatic 
> runs in a few different ways.  To opt out
> completely, a contributor can simply disable GitHub Actions on their personal 
> fork, and no further jobs will be
> executed. Another option is to add a repository secret [4] with the name 
> `JDK_SUBMIT_FILTER` set to any value. If this
> is set, only branches prefixed with `submit/` will be subject to automatic 
> build and test. This can also be further
> refined by adding a repository secret named `JDK_SUBMIT_PLATFORMS` with a 
> value such as `Linux x64, Windows x64` to
> limit automatic build and test to these two platforms. It will still be 
> possible to run the tests on any branch and/or
> platform by manually triggering the workflow.  To see what this looks like in 
> practice, an example run can be found
> here: https://github.com/rwestberg/jdk/actions/runs/265131985 (note that 
> there is currently a failing test on Windows
> which is tracked by JDK-8249095, which should probably be resolved before 
> this change is integrated).  Best regards,
> Robin  [1] 
> https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html [2]
> https://github.com/features/actions [3]
> https://docs.github.com/en/actions/getting-started-with-github-actions/about-github-actions#usage-limits
>  [4]
> https://docs.github.com/en/actions/reference/encrypted-secrets

Robin Westberg has updated the pull request incrementally with one additional 
commit since the last revision:

  Extract all hardcoded versions to a separate configuration file

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/284/files
  - new: https://git.openjdk.java.net/jdk/pull/284/files/4074e334..7f260ede

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=284=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=284=00-01

  Stats: 170 lines in 2 files changed: 102 ins; 5 del; 63 mod
  Patch: https://git.openjdk.java.net/jdk/pull/284.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/284/head:pull/284

PR: https://git.openjdk.java.net/jdk/pull/284

Re: RFR: 8253424: Add support for running pre-submit testing using GitHub Actions

[Robin Westberg]

On Tue, 22 Sep 2020 14:05:42 GMT, Erik Joelsson  wrote:

>> Sure, should not be that hard to parse something similar. The GitHub actions 
>> will probably need it in JSON format at
>> some point, but nothing a little `sed -e '1i {' -e 's/#.*//g' -e 's/"//g' -e 
>> 's/(.*)=(.*)/"\1": "\2",/g' -e
>> '$s/,\s{0,}$/}/'` won't solve. Any suggestion for the location and name of 
>> such a file? I'm thinking it would contain
>> entries like this:  BOOT_JDK_VERSION="14.0.2" JTREG_VERSION="jtreg5.1-b01"
>> GTEST_VERSION="release-1.8.1"
>> LINUX_X64_BOOT_JDK_FILENAME="openjdk-14.0.2_linux-x64_bin.tar.gz"
>> LINUX_X64_BOOT_JDK_URL="https://download.java.net/java/GA/jdk14.0.2/205943a0976c4ed48cb16f1043c5c647/12/GPL/openjdk-14.0.2_linux-x64_bin.tar.gz;
>> LINUX_X64_BOOT_JDK_SHA256="91310200f072045dc6cef2c8c23e7e6387b37c46e9de49623ce0fa461a24623d"
>
> The other primary consumer of this is make/conf/jib-profiles.js. The 
> make/conf dir would make sense to me.
> 
> The challenge here is creating a set of variables that are suitable enough 
> for both config files to consume. For
> BootJDK, we never bothered with bumping the version for updates, and I very 
> much doubt we will do that in the future
> for github actions, so a plain major version 14, and soon 15, would be 
> preferred from my point of view. This is however
> not enough for jib-profiles.js (yet) so we can't really share bootjdk config 
> for now anyway. For jtreg, we specify 5.1
> and b01 as two separate metadata values. For gtest we specify the version as 
> 1.8.1.

I added `make/conf/test-dependencies` with version numbers specified on the 
format that `jib-profiles.js` would expect.
Actually using them from that file as well could perhaps be a separate task 
though. :)

-

PR: https://git.openjdk.java.net/jdk/pull/284

Re: RFR: 8253424: Add support for running pre-submit testing using GitHub Actions

[Robin Westberg]

On Mon, 21 Sep 2020 17:22:57 GMT, Erik Joelsson  wrote:

>> Certainly, the prerequisites step can checkout additional repositories and 
>> run shell commands to extract variables to
>> be used for later steps. Do we have any suitable source for these versions? 
>> We'll also need download links for the
>> different flavors of the bootjdk (and ideally the sha256 hash, although the 
>> verification step could perhaps be skipped).
>
> We don't have a good source currently no, but something to keep in mind. 
> Ideally we would want to create a common
> source for both jib-profiles.js and the gitactions to read from. I would 
> prefer something that can be read both as a
> properties file and sourced as a shell script, much like the version-numbers 
> file currently is. That could be done as a
> separate change though.

Sure, should not be that hard to parse something similar. The GitHub actions 
will probably need it in JSON format at
some point, but nothing a little `sed -e '1i {' -e 's/#.*//g' -e 's/"//g' -e 
's/(.*)=(.*)/"\1": "\2",/g' -e
'$s/,\s{0,}$/}/'` won't solve. Any suggestion for the location and name of such 
a file? I'm thinking it would contain
entries like this:

BOOT_JDK_VERSION="14.0.2"
JTREG_VERSION="jtreg5.1-b01"
GTEST_VERSION="release-1.8.1"
LINUX_X64_BOOT_JDK_FILENAME="openjdk-14.0.2_linux-x64_bin.tar.gz"
LINUX_X64_BOOT_JDK_URL="https://download.java.net/java/GA/jdk14.0.2/205943a0976c4ed48cb16f1043c5c647/12/GPL/openjdk-14.0.2_linux-x64_bin.tar.gz;
LINUX_X64_BOOT_JDK_SHA256="91310200f072045dc6cef2c8c23e7e6387b37c46e9de49623ce0fa461a24623d"

-

PR: https://git.openjdk.java.net/jdk/pull/284

Re: RFR: 8253424: Add support for running pre-submit testing using GitHub Actions

[Robin Westberg]

On Mon, 21 Sep 2020 13:59:06 GMT, Erik Joelsson  wrote:

>> A few days ago I posted an initial version of the necessary configuration 
>> required to run pre-submit build and tests
>> for JDK main-line contributions using GitHub Actions [2] and the free tier 
>> [3] available to everyone working with open
>> source repositories. I've incorporated the feedback into an updated version 
>> that I believe is ready to be integrated.
>> If this is integrated into the `master` branch, future branches created and 
>> updated in personal forks will build and
>> run the basic tier 1 tests as described in this configuration, on Linux, 
>> Windows and macOS (all on x64). It's of course
>> possible for any contributor to opt out fully or partially of these 
>> automatic runs in a few different ways.  To opt out
>> completely, a contributor can simply disable GitHub Actions on their 
>> personal fork, and no further jobs will be
>> executed. Another option is to add a repository secret [4] with the name 
>> `JDK_SUBMIT_FILTER` set to any value. If this
>> is set, only branches prefixed with `submit/` will be subject to automatic 
>> build and test. This can also be further
>> refined by adding a repository secret named `JDK_SUBMIT_PLATFORMS` with a 
>> value such as `Linux x64, Windows x64` to
>> limit automatic build and test to these two platforms. It will still be 
>> possible to run the tests on any branch and/or
>> platform by manually triggering the workflow.  To see what this looks like 
>> in practice, an example run can be found
>> here: https://github.com/rwestberg/jdk/actions/runs/265131985 (note that 
>> there is currently a failing test on Windows
>> which is tracked by JDK-8249095, which should probably be resolved before 
>> this change is integrated).  Best regards,
>> Robin  [1] 
>> https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html 
>> [2]
>> https://github.com/features/actions [3]
>> https://docs.github.com/en/actions/getting-started-with-github-actions/about-github-actions#usage-limits
>>  [4]
>> https://docs.github.com/en/actions/reference/encrypted-secrets
>
> Is there any way to derive the version strings for dependencies from a 
> different source? Once pushed, this file will
> require updating whenever the bootjdk, jtreg version or gtest version is 
> updated.

Certainly, the prerequisites step can checkout additional repositories and run 
shell commands to extract variables to
be used for later steps. Do we have any suitable source for these versions? 
We'll also need download links for the
different flavors of the bootjdk (and ideally the sha256 hash, although the 
verification step could perhaps be skipped).

-

PR: https://git.openjdk.java.net/jdk/pull/284

RFR: 8253424: Add support for running pre-submit testing using GitHub Actions

[Robin Westberg]

A few days ago I posted an initial version of the necessary configuration 
required to run pre-submit build and tests
for JDK main-line contributions using GitHub Actions [2] and the free tier [3] 
available to everyone working with open
source repositories. I've incorporated the feedback into an updated version 
that I believe is ready to be integrated.

If this is integrated into the `master` branch, future branches created and 
updated in personal forks will build and
run the basic tier 1 tests as described in this configuration, on Linux, 
Windows and macOS (all on x64). It's of course
possible for any contributor to opt out fully or partially of these automatic 
runs in a few different ways.

To opt out completely, a contributor can simply disable GitHub Actions on their 
personal fork, and no further jobs will
be executed. Another option is to add a repository secret [4] with the name 
`JDK_SUBMIT_FILTER` set to any value. If
this is set, only branches prefixed with `submit/` will be subject to automatic 
build and test. This can also be
further refined by adding a repository secret named `JDK_SUBMIT_PLATFORMS` with 
a value such as `Linux x64, Windows
x64` to limit automatic build and test to these two platforms. It will still be 
possible to run the tests on any branch
and/or platform by manually triggering the workflow.

To see what this looks like in practice, an example run can be found here:
https://github.com/rwestberg/jdk/actions/runs/265131985 (note that there is 
currently a failing test on Windows which
is tracked by JDK-8249095, which should probably be resolved before this change 
is integrated).

Best regards,
Robin

[1] https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html
[2] https://github.com/features/actions
[3] 
https://docs.github.com/en/actions/getting-started-with-github-actions/about-github-actions#usage-limits
[4] https://docs.github.com/en/actions/reference/encrypted-secrets

-

Commit messages:
 - Updates after initial preview
 - Initial version

Changes: https://git.openjdk.java.net/jdk/pull/284/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=284=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8253424
  Stats: 739 lines in 1 file changed: 739 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/284.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/284/head:pull/284

PR: https://git.openjdk.java.net/jdk/pull/284

Integrated: 8252897: Minor .jcheck/conf update

[Robin Westberg]

On Tue, 8 Sep 2020 10:31:42 GMT, Robin Westberg  wrote:

> The initial version of the Skara-formatted jcheck configuration file 
> contained a small error, this change corrects it.
> 
> Best regards,
> Robin

This pull request has now been integrated.

Changeset: bf5da0c7
Author:    Robin Westberg 
URL:   https://git.openjdk.java.net/jdk/commit/bf5da0c7
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8252897: Minor .jcheck/conf update

Reviewed-by: ehelin

-

PR: https://git.openjdk.java.net/jdk/pull/70

Re: RFR: 8252897: Minor .jcheck/conf update

[Robin Westberg]

On Tue, 8 Sep 2020 11:41:31 GMT, Erik Helin  wrote:

>> The initial version of the Skara-formatted jcheck configuration file 
>> contained a small error, this change corrects it.
>> 
>> Best regards,
>> Robin
>
> Looks good, thanks for fixing!

Thanks for reviewing Erik! As this problem affects other pull requests, I'm 
going to integrate it right away.

-

PR: https://git.openjdk.java.net/jdk/pull/70

RFR: 8252897: Minor .jcheck/conf update

[Robin Westberg]

The initial version of the Skara-formatted jcheck configuration file contained 
a small error, this change corrects it.

Best regards,
Robin

-

Commit messages:
 - Fix mistake in jcheck configuration file

Changes: https://git.openjdk.java.net/jdk/pull/70/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=70=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252897
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/70.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/70/head:pull/70

PR: https://git.openjdk.java.net/jdk/pull/70

Integrated: 8252844: Update check configuration to Skara format

[Robin Westberg]

On Mon, 7 Sep 2020 07:19:50 GMT, Robin Westberg  wrote:

> Now that the JDK main-line repository is using the Skara tooling, the jcheck 
> configuration file should be updated to
> the new format.
> Best regards,
> Robin

This pull request has now been integrated.

Changeset: e0c8d442
Author:    Robin Westberg 
URL:   https://git.openjdk.java.net/jdk/commit/e0c8d442
Stats: 29 lines in 1 file changed: 0 ins; 28 del; 1 mod

8252844: Update check configuration to Skara format

Reviewed-by: ehelin

-

PR: https://git.openjdk.java.net/jdk/pull/39

Re: RFR: 8252844: Update check configuration to Skara format [v2]

[Robin Westberg]

On Mon, 7 Sep 2020 08:01:54 GMT, Erik Helin  wrote:

>> Robin Westberg has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Apply review suggestion
>>   
>>   Co-authored-by: Erik Duveblad 
>
> Looks good!

Thanks for reviewing Erik! As this is blocking other pull requests from 
completing, I plan to integrate it right away.

-

PR: https://git.openjdk.java.net/jdk/pull/39

Re: RFR: 8252844: Update check configuration to Skara format [v2]

[Robin Westberg]

> Now that the JDK main-line repository is using the Skara tooling, the jcheck 
> configuration file should be updated to
> the new format.
> Best regards,
> Robin

Robin Westberg has updated the pull request incrementally with one additional 
commit since the last revision:

  Apply review suggestion
  
  Co-authored-by: Erik Duveblad 

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/39/files
  - new: https://git.openjdk.java.net/jdk/pull/39/files/ee32112a..a23e3d6f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=39=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=39=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/39.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/39/head:pull/39

PR: https://git.openjdk.java.net/jdk/pull/39

RFR: 8252844: Update check configuration to Skara format

[Robin Westberg]

Now that the JDK main-line repository is using the Skara tooling, the jcheck 
configuration file should be updated to
the new format.

Best regards,
Robin

-

Commit messages:
 - Initial update

Changes: https://git.openjdk.java.net/jdk/pull/39/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=39=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252844
  Stats: 29 lines in 1 file changed: 28 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/39.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/39/head:pull/39

PR: https://git.openjdk.java.net/jdk/pull/39

Re: RFR: 8223678: Add Visual Studio Code workspace generation support (for native code)

[Robin Westberg]

Hi Erik,

> On 31 May 2019, at 18:15, Erik Joelsson  wrote:
> 
> Hello Robin,
> 
> On 2019-05-31 05:26, Robin Westberg wrote:
>> Hi Erik,
>> 
>>> On 29 May 2019, at 17:22, Erik Joelsson  wrote:
>>> 
>>> Thanks, looks good!
>> Thanks for reviewing! Unfortunately I had to do a few more general changes 
>> to get the “ccls" indexer working properly as well, hope you don’t mind 
>> taking another look:
>> 
>> Webrevs:
>>  - Full: http://cr.openjdk.java.net/~rwestberg/8223678/webrev.03/
>>  - Inc: http://cr.openjdk.java.net/~rwestberg/8223678/webrev.02-03/
> 
> Looks good!
> 
> I sure appreciate adding tests when modifying the hairier utility macros.

Thanks again for reviewing!

I did a small tweak to the settings of one of the optional indexers (ccls), 
perhaps not something that needs a detailed review, but here’s the final 
version that I’m thinking of pushing:

Webrev:
 - Full: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.04/
 - Inc: http://cr.openjdk.java.net/~rwestberg/8223678/webrev.03-04/

Best regards,
Robin

> 
> /Erik
> 
>> Best regards,
>> Robin
>> 
>>> /Erik
>>> 
>>> On 2019-05-29 06:52, Robin Westberg wrote:
>>>> Hi Erik,
>>>> 
>>>> Thanks for taking a look!
>>>> 
>>>>> On 28 May 2019, at 15:52, Erik Joelsson  wrote:
>>>>> 
>>>>> Hello Robin,
>>>>> 
>>>>> Looks good.
>>>>> 
>>>>> Adding of ide.md is very nice, but could you try to format it like 
>>>>> testing.md and building.md in regards to line lengths? I believe we are 
>>>>> trying for 80 chars in those to make them read reasonably in a standard 
>>>>> editor.
>>>> Sure, did a bit of reflowing and updated some formatting in order to look 
>>>> more like the other .md files.
>>>> 
>>>> Webrevs:
>>>>  - Full: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.02/
>>>>  - Inc: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01-02/
>>>> 
>>>> Best regards,
>>>> Robin
>>>> 
>>>>> /Erik
>>>>> 
>>>>> On 2019-05-27 09:03, Robin Westberg wrote:
>>>>>> Hi all,
>>>>>> 
>>>>>> Please review this change that adds build system support for generating 
>>>>>> a Visual Studio Code workspace configured for working with the JDK 
>>>>>> native code. It configures the default C/C++ IntelliSense Engine to 
>>>>>> allow code completion/navigation and similar features. It also 
>>>>>> configures two executable targets (gtestLauncher and java) that can be 
>>>>>> built and debugged from the IDE.
>>>>>> 
>>>>>> The main target is "make vscode-project”, additional information can be 
>>>>>> found in doc/ide.[md|html].
>>>>>> 
>>>>>> Issue:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8223678
>>>>>> 
>>>>>> Webrev:
>>>>>> https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01/
>>>>>> 
>>>>>> Testing:
>>>>>> Manual testing on Linux, MacOS and Windows
>>>>>> 
>>>>>> Thanks Erik Joelsson for taking a look at an earlier version of it!
>>>>>> 
>>>>>> Best regards,
>>>>>> Robin

Re: RFR: 8223678: Add Visual Studio Code workspace generation support (for native code)

[Robin Westberg]

Hi Volker,

> On 29 May 2019, at 16:01, Volker Simonis  wrote:
> 
> On Wed, May 29, 2019 at 3:43 PM Robin Westberg
>  wrote:
>> 
>> Hi Volker,
>> 
>>> On 28 May 2019, at 17:33, Volker Simonis  wrote:
>>> 
>>> Hi Robin,
>>> 
>>> thanks a lot for doing this!
>>> 
>>> I have just a quick question. Do you know if any of the VSC indexers
>>> (default, clangd) support call hierarchies (i.e. "called by",
>>> "callers" of a function/method) and "used by" for variables/class
>>> fields?
>> 
>> Sure, I can make a quick summary of the various pros and cons of the 
>> indexers that I’ve found so far. They are all moving pretty fast though, so 
>> didn’t think it was a good fit for the documentation file.
>> 
>> In general, VS Code itself only just recently gained proper support for 
>> displaying call hierarchies: 
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__code.visualstudio.com_updates_v1-5F33-23-5Fcall-2Dhierarchy=DwIFaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=UJlIwvo0Thp_BYS_NWFT0ryBTIkcL2KhsFz8CKsa4GY=82pYT4_vb0KoCg2J7f8ZHebXioM2IbSDUpVgSuUAz-A=yY4u7WPyw8IMAQOytIDpU3MePzQMuGgCTyIP4XuHXbI=
>>  - but alternative indexers have worked around this by showing them 
>> differently.
>> 
>> Default (Microsoft - C/C++ for Visual Studio Code)
>> + Easy to setup, as no additional dependencies are needed
>> + Good “go to definition”, the only one that can make sense of the 
>> template+macro stuff in log.hpp for example
>> - Find references (used by) not done yet (said to be coming in the June 
>> update: 
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_microsoft_vscode-2Dcpptools_issues_15=DwIFaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=UJlIwvo0Thp_BYS_NWFT0ryBTIkcL2KhsFz8CKsa4GY=82pYT4_vb0KoCg2J7f8ZHebXioM2IbSDUpVgSuUAz-A=usQi8Sluzbg7sm51fLwBgFVSkY8uj_ZkOgFhT6clIjw=)
>> - Call hierarchies also not there (scheduled for September apparently: 
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_microsoft_vscode-2Dcpptools_issues_16=DwIFaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=UJlIwvo0Thp_BYS_NWFT0ryBTIkcL2KhsFz8CKsa4GY=82pYT4_vb0KoCg2J7f8ZHebXioM2IbSDUpVgSuUAz-A=0nJoexh1jJuqrNQ5bhxtpwdzS9LhGYhzKyEmFbM1D8A=)
>> 
>> clangd
>> + Actively developed and part of the llvm/clang project
>> + Support for find references
>> - ..but not call hierarchies yet
>> - Full project indexing is still flagged as experimental, and currently 
>> seems to fail when editing commonly used header files for example
>> 
>> Full feature list: 
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__clang.llvm.org_extra_clangd_Features.html-23complete-2Dlist-2Dof-2Dfeatures=DwIFaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=UJlIwvo0Thp_BYS_NWFT0ryBTIkcL2KhsFz8CKsa4GY=82pYT4_vb0KoCg2J7f8ZHebXioM2IbSDUpVgSuUAz-A=-59_vkfX4TV8qO7PoHopjLBC1c2Qu-HjneQnNM49Zps=
>> 
>> rtags
>> This is currently the most feature-complete indexer I think. But the VS Code 
>> integration is a bit minimal and not part of the rtags project itself, and 
>> it is missing things like indexer progress.
>> + The indexer is actively developed and has been around for quite some time
>> - You will probably have to build the indexer from source
>> + Full support for call hierarchies and find references
>> - VS Code integration a bit limited, missing convenience features like 
>> switch between header/source file, showing method/class documentation on 
>> hover.
>> 
>> There are even more indexers of course, a promising one used to be “cquery", 
>> but that project seems to be defunct now. It lives on in the “ccls" indexer, 
>> but ithat one is a bit tricky to configure unless you use clang for building 
>> the JDK as well.
>> 
>> So in summary, after the summer the default indexer might be the obvious 
>> best choice, but right now it depends on which features you think are the 
>> most important I guess..
>> 
> 
> Hi Robin,
> 
> thanks a lot for the great summary! Incidentally I've just did some
> Google research as well and basically came to the same conclusion.
> "Cquery" seemed quite promising and also pretends to have call
> hierarchies. But it seems to be defunct and I've also found some bug
> reports about problems with the call hierarchies.

Yeah, cquery did work okay when I tried it, but had some issues. However, the 
“spiritual” successor ccls has fixed all the issues I had with cquery, I spent 
a bit more time on configuring it today and finally figured out why it didn’t 
work!

> For me "Called Hierarchy", and &quo

Re: RFR: 8223678: Add Visual Studio Code workspace generation support (for native code)

[Robin Westberg]

Hi Erik,

> On 29 May 2019, at 17:22, Erik Joelsson  wrote:
> 
> Thanks, looks good!

Thanks for reviewing! Unfortunately I had to do a few more general changes to 
get the “ccls" indexer working properly as well, hope you don’t mind taking 
another look:

Webrevs:
 - Full: http://cr.openjdk.java.net/~rwestberg/8223678/webrev.03/
 - Inc: http://cr.openjdk.java.net/~rwestberg/8223678/webrev.02-03/

Best regards,
Robin

> 
> /Erik
> 
> On 2019-05-29 06:52, Robin Westberg wrote:
>> Hi Erik,
>> 
>> Thanks for taking a look!
>> 
>>> On 28 May 2019, at 15:52, Erik Joelsson  wrote:
>>> 
>>> Hello Robin,
>>> 
>>> Looks good.
>>> 
>>> Adding of ide.md is very nice, but could you try to format it like 
>>> testing.md and building.md in regards to line lengths? I believe we are 
>>> trying for 80 chars in those to make them read reasonably in a standard 
>>> editor.
>> Sure, did a bit of reflowing and updated some formatting in order to look 
>> more like the other .md files.
>> 
>> Webrevs:
>>  - Full: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.02/
>>  - Inc: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01-02/
>> 
>> Best regards,
>> Robin
>> 
>>> /Erik
>>> 
>>> On 2019-05-27 09:03, Robin Westberg wrote:
>>>> Hi all,
>>>> 
>>>> Please review this change that adds build system support for generating a 
>>>> Visual Studio Code workspace configured for working with the JDK native 
>>>> code. It configures the default C/C++ IntelliSense Engine to allow code 
>>>> completion/navigation and similar features. It also configures two 
>>>> executable targets (gtestLauncher and java) that can be built and debugged 
>>>> from the IDE.
>>>> 
>>>> The main target is "make vscode-project”, additional information can be 
>>>> found in doc/ide.[md|html].
>>>> 
>>>> Issue:
>>>> https://bugs.openjdk.java.net/browse/JDK-8223678
>>>> 
>>>> Webrev:
>>>> https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01/
>>>> 
>>>> Testing:
>>>> Manual testing on Linux, MacOS and Windows
>>>> 
>>>> Thanks Erik Joelsson for taking a look at an earlier version of it!
>>>> 
>>>> Best regards,
>>>> Robin

Re: RFR: 8223678: Add Visual Studio Code workspace generation support (for native code)

[Robin Westberg]

Hi Erik,

Thanks for taking a look!

> On 28 May 2019, at 15:52, Erik Joelsson  wrote:
> 
> Hello Robin,
> 
> Looks good.
> 
> Adding of ide.md is very nice, but could you try to format it like testing.md 
> and building.md in regards to line lengths? I believe we are trying for 80 
> chars in those to make them read reasonably in a standard editor.

Sure, did a bit of reflowing and updated some formatting in order to look more 
like the other .md files.

Webrevs:
 - Full: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.02/
 - Inc: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01-02/

Best regards,
Robin

> 
> /Erik
> 
> On 2019-05-27 09:03, Robin Westberg wrote:
>> Hi all,
>> 
>> Please review this change that adds build system support for generating a 
>> Visual Studio Code workspace configured for working with the JDK native 
>> code. It configures the default C/C++ IntelliSense Engine to allow code 
>> completion/navigation and similar features. It also configures two 
>> executable targets (gtestLauncher and java) that can be built and debugged 
>> from the IDE.
>> 
>> The main target is "make vscode-project”, additional information can be 
>> found in doc/ide.[md|html].
>> 
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8223678
>> 
>> Webrev:
>> https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01/
>> 
>> Testing:
>> Manual testing on Linux, MacOS and Windows
>> 
>> Thanks Erik Joelsson for taking a look at an earlier version of it!
>> 
>> Best regards,
>> Robin

Re: RFR: 8223678: Add Visual Studio Code workspace generation support (for native code)

[Robin Westberg]

Hi Volker,

> On 28 May 2019, at 17:33, Volker Simonis  wrote:
> 
> Hi Robin,
> 
> thanks a lot for doing this!
> 
> I have just a quick question. Do you know if any of the VSC indexers
> (default, clangd) support call hierarchies (i.e. "called by",
> "callers" of a function/method) and "used by" for variables/class
> fields?

Sure, I can make a quick summary of the various pros and cons of the indexers 
that I’ve found so far. They are all moving pretty fast though, so didn’t think 
it was a good fit for the documentation file.

In general, VS Code itself only just recently gained proper support for 
displaying call hierarchies: 
https://code.visualstudio.com/updates/v1_33#_call-hierarchy - but alternative 
indexers have worked around this by showing them differently. 

Default (Microsoft - C/C++ for Visual Studio Code)
+ Easy to setup, as no additional dependencies are needed
+ Good “go to definition”, the only one that can make sense of the 
template+macro stuff in log.hpp for example
- Find references (used by) not done yet (said to be coming in the June update: 
https://github.com/microsoft/vscode-cpptools/issues/15)
- Call hierarchies also not there (scheduled for September apparently: 
https://github.com/microsoft/vscode-cpptools/issues/16)

clangd
+ Actively developed and part of the llvm/clang project
+ Support for find references
- ..but not call hierarchies yet
- Full project indexing is still flagged as experimental, and currently seems 
to fail when editing commonly used header files for example

Full feature list: 
https://clang.llvm.org/extra/clangd/Features.html#complete-list-of-features

rtags
This is currently the most feature-complete indexer I think. But the VS Code 
integration is a bit minimal and not part of the rtags project itself, and it 
is missing things like indexer progress.
+ The indexer is actively developed and has been around for quite some time
- You will probably have to build the indexer from source
+ Full support for call hierarchies and find references
- VS Code integration a bit limited, missing convenience features like switch 
between header/source file, showing method/class documentation on hover.

There are even more indexers of course, a promising one used to be “cquery", 
but that project seems to be defunct now. It lives on in the “ccls" indexer, 
but ithat one is a bit tricky to configure unless you use clang for building 
the JDK as well.

So in summary, after the summer the default indexer might be the obvious best 
choice, but right now it depends on which features you think are the most 
important I guess..

Best regards,
Robin

> 
> Regards,
> Volker
> 
> On Mon, May 27, 2019 at 6:03 PM Robin Westberg
>  wrote:
>> 
>> Hi all,
>> 
>> Please review this change that adds build system support for generating a 
>> Visual Studio Code workspace configured for working with the JDK native 
>> code. It configures the default C/C++ IntelliSense Engine to allow code 
>> completion/navigation and similar features. It also configures two 
>> executable targets (gtestLauncher and java) that can be built and debugged 
>> from the IDE.
>> 
>> The main target is "make vscode-project”, additional information can be 
>> found in doc/ide.[md|html].
>> 
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8223678
>> 
>> Webrev:
>> https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01/
>> 
>> Testing:
>> Manual testing on Linux, MacOS and Windows
>> 
>> Thanks Erik Joelsson for taking a look at an earlier version of it!
>> 
>> Best regards,
>> Robin

RFR: 8223678: Add Visual Studio Code workspace generation support (for native code)

[Robin Westberg]

Hi all,

Please review this change that adds build system support for generating a 
Visual Studio Code workspace configured for working with the JDK native code. 
It configures the default C/C++ IntelliSense Engine to allow code 
completion/navigation and similar features. It also configures two executable 
targets (gtestLauncher and java) that can be built and debugged from the IDE.

The main target is "make vscode-project”, additional information can be found 
in doc/ide.[md|html].

Issue:
https://bugs.openjdk.java.net/browse/JDK-8223678

Webrev:
https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01/

Testing:
Manual testing on Linux, MacOS and Windows

Thanks Erik Joelsson for taking a look at an earlier version of it! 

Best regards,
Robin

Re: RFR(L/XS) : 8222414 : bring googlemock v1.8.1

[Robin Westberg]

Hi Igor,

Looks good to me, I tried rewriting parts of an existing test to use gmock 
instead of a handcrafted mock, and things worked as expected!

Best regards,
Robin

> On 24 May 2019, at 23:33, Igor Ignatyev  wrote:
> 
> @Erik, Thanks!
> 
> @hotspot (looking at Robin), can I get another review? 
> 
> -- Igor
> 
>> On May 24, 2019, at 8:28 AM, Erik Joelsson  wrote:
>> 
>> Thanks, looks good!
>> 
>> /Erik
>> 
>> On 2019-05-23 19:07, Igor Ignatyev wrote:
>>> Hi Erik,
>>> 
>>> thanks for your review. I've updated CompileGtest.gmk as you advised[1], 
>>> which gives [2].
>>> 
>>> -- Igor
>>> 
>>> [1]
 diff -r 3443e083223d make/hotspot/lib/CompileGtest.gmk
 --- a/make/hotspot/lib/CompileGtest.gmk Thu May 23 19:03:32 2019 -0700
 +++ b/make/hotspot/lib/CompileGtest.gmk Thu May 23 19:04:09 2019 -0700
 @@ -64,8 +64,9 @@
 EXCLUDES := $(JVM_EXCLUDES), \
 EXCLUDE_FILES := gtestLauncher.cpp, \
 EXCLUDE_PATTERNS := $(JVM_EXCLUDE_PATTERNS), \
 -EXTRA_FILES := $(GTEST_FRAMEWORK_SRC)/googletest/src/gtest-all.cc \
 -   $(GTEST_FRAMEWORK_SRC)/googlemock/src/gmock-all.cc, \
 +EXTRA_FILES := \
 +$(GTEST_FRAMEWORK_SRC)/googletest/src/gtest-all.cc \
 +$(GTEST_FRAMEWORK_SRC)/googlemock/src/gmock-all.cc, \
 EXTRA_OBJECT_FILES := $(filter-out %/operator_new$(OBJ_SUFFIX), \
 $(BUILD_LIBJVM_ALL_OBJS)), \
 CFLAGS := $(JVM_CFLAGS) \
>>> [2] http://cr.openjdk.java.net/~iignatyev/8222414/webrev.1-2/
>>> 
 On May 23, 2019, at 6:57 AM, Erik Joelsson  
 wrote:
 
 Hello Igor,
 
 Build changes look ok, with a (very) minor style suggestion [1] (point 
 18). We try to avoid padding continuation line breaks, so I would 
 appreciate if CompileGtest.gmk:68 could be indented just 4 extra spaces 
 compared to line 67. If you want the file names to line up, it's 
 acceptable to break after the := on 67.
 
 [1] http://openjdk.java.net/groups/build/doc/code-conventions.html
 
 /Erik
 
 On 2019-05-22 22:20, Igor Ignatyev wrote:
> http://cr.openjdk.java.net/~iignatyev//8222414/webrev.01
>> 21638 lines changed: 21628 ins; 0 del; 10 mod;
> Hi all,
> 
> could you please review this patch which makes mocking framework from 
> google test available for hotspot tests?
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8222414
> testing: tier1
> webrevs:
> - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.00
>> 21605 lines changed: 21605 ins; 0 del; 0 mod;
> * moves gtest from test/fmw/gtest to test/fmw/gtest/googletest;
> * adds only required (w/o build-aux, docs, make, msvc, scripts, tests 
> directories and make related files) source of gmock to 
> test/fmw/gtest/googlemock;
> 
> - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.0-1
>> 33 lines changed: 23 ins; 0 del; 10 mod;
> * makes necessary changes in makefiles
> * calls gmock framework init function
> * works around the conflict b/w :testing::internal::Log function defined 
> by gmock and Log macro defined by hotspot
> 
> - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.01
>> 21638 lines changed: 21628 ins; 0 del; 10 mod;
> all the changes together
> 
> 
> thanks,
> -- Igor
> 

RFR: JDK-8218807: Compilation database (compile_commands.json) may contain obsolete items

[Robin Westberg]

Hi all,

When generating a compilation database by either "make compile-commands" or 
"make compile-commands-hotspot", every object file that should be built results 
in a temporary json fragment describing the compiler invocation. However, when 
the final compile_commands.json file is assembled, all these temporary files 
are combined, regardless of whether they belong to the current make invocation 
or were left behind from a previous one. 

This has the unfortunate effect that "make compile-commands" followed by "make 
compile-commands-hotspot" generates something very different from an initial 
invocation of "make compile-commands-hotspot". Also, if a source file has been 
removed, it will still retain an entry in the final compile_commands.json file 
until the build directory is cleaned. This will lead to errors if generating an 
IDE project from the compile_commands.json file. 

Proposed solution is to always remove all previous temporary json fragments and 
generate them again. This generation is quite fast and only adds a second or 
two for repeated invocations of "make compile-commands”, which should be a 
reasonable tradeoff for ensuring that the compilation database contains correct 
information.

Issue: https://bugs.openjdk.java.net/browse/JDK-8218807
Webrev: http://cr.openjdk.java.net/~rwestberg/8218807/
Testing: make test-make-compile-commands, build windows, linux, mac, solaris

Best regards,
Robin

Re: Is there a cmakelists.txt file of openjdk11

[Robin Westberg]

(moving to build-dev)

Hi,

As of version 2018.2 [1], as an alternative to CMakeLists.txt, CLion can make 
use of compile_commands.json [2] as well. After configuring your build, you can 
generate such a file with the JDK make system:

$ make compile-commands

Or alternatively, if you are only interested in hotspot sources:

$ make compile-commands-hotspot

The compile_commands.json file will be created in the build// folder. 
Simply opening that folder from CLion should work fine, but you may want to 
edit some project properties afterwards to improve the appearance of the source 
tree listing.

Best regards,
Robin

[1]: 
https://blog.jetbrains.com/clion/2018/08/working-with-makefiles-in-clion-using-compilation-db/
[2]: http://clang.llvm.org/docs/JSONCompilationDatabase.html

> On 18 Dec 2018, at 04:33, David Holmes  wrote:
> 
> I should add that this was raised in the past. Not sure what happened to it:
> 
> http://mail.openjdk.java.net/pipermail/build-dev/2017-March/018846.html
> 
> but again, take this up on build-dev.
> 
> David
> 
> On 18/12/2018 12:36 pm, David Holmes wrote:
>> Hi,
>> On 18/12/2018 12:29 pm, 毛宝龙 wrote:
>>> Hi all, we need cmakelists.txt to make ability of clion could be the ide 
>>> for openjdk. Because clion support cmake only, but not makefile project. In 
>>> some famous c comunity, cmake is more and more popular. So is there a 
>>> cmakelists.txt?
>> No there isn't. I suggest discussing on build-dev@openjdk.java.net.
>> Cheers,
>> David
>>> 
>>> 
>>> 
>>> 

Re: RFR: 8212004: Optional compile_commands.json field not compatible with older libclang

[Robin Westberg]

Hi Erik,

Thanks for reviewing!

Best regards,
Robin

> On 10 Oct 2018, at 17:34, Erik Joelsson  wrote:
> 
> Looks good.
> 
> /Erik
> 
> 
> On 2018-10-10 07:02, Robin Westberg wrote:
>> Hi all,
>> 
>> Please review this small change to remove the “output” field from the 
>> compile_commands.json file generated by “make compile-commands”. As it turns 
>> out, this optional field is incompatible with tooling built on libclang 
>> versions older than 4.0. As this field provides no real benefit for us, it 
>> can simply be dropped from the output.
>> 
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8212004
>> Webrev: https://cr.openjdk.java.net/~rwestberg/8212004/webrev.00/
>> Testing: tier1, builds-tier5
>> 
>> Best regards,
>> Robin
> 

RFR: 8212004: Optional compile_commands.json field not compatible with older libclang

[Robin Westberg]

Hi all,

Please review this small change to remove the “output” field from the 
compile_commands.json file generated by “make compile-commands”. As it turns 
out, this optional field is incompatible with tooling built on libclang 
versions older than 4.0. As this field provides no real benefit for us, it can 
simply be dropped from the output.

Issue: https://bugs.openjdk.java.net/browse/JDK-8212004
Webrev: https://cr.openjdk.java.net/~rwestberg/8212004/webrev.00/
Testing: tier1, builds-tier5

Best regards,
Robin

Re: RFR: 8210459: Add support for generating compile_commands.json

[Robin Westberg]

Hi Erik & Magnus,

Thanks for the final reviews, pushed it with the suggested changes.

Best regards,
Robin

> On 4 Oct 2018, at 18:24, Erik Joelsson  wrote:
> 
> Looks good.
> 
> Minor style issue, the broken find line should be indented 4 spaces for 
> continuation. Strictly speaking so should the awk program lines be in this 
> case. No need for respin.
> 
> /Erik
> 
> 
> On 2018-10-04 06:31, Magnus Ihse Bursie wrote:
>>> 4 okt. 2018 kl. 15:03 skrev Robin Westberg :
>>> 
>>> Hi Magnus,
>>> 
>>> Thanks again for reviewing!
>>> 
>>>>> On 4 Oct 2018, at 13:38, Magnus Ihse Bursie 
>>>>>  wrote:
>>>>> 
>>>>> Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.04/
>>>> I was about to say that this looks good, but then I discovered a weird 
>>>> thing. I'm the SED line, line 50, there is a weird C-like Unicode 
>>>> character instead of a quote, after -e. Looks really odd. Is it a "smart 
>>>> quote" gone wrong? Can you look into it and fix it? You don't need to 
>>>> respin the webrev for that. Otherwise, you're all good to go.
>>> Ah indeed, nice catch! Fortunately it seems to be an issue with the 
>>> “git-webrev” tool, there’s a missing semicolon in the html’ized version: 
>>> $(SED) -e s/^/[. (The ‘raw’ view for example is correct).
>> Good.
>> 
>>> I did notice one more thing that I was hoping to sneak into an in-place 
>>> update: the raw ‘cat’ on line 47 should probably be $(CAT) as well, right? 
>>> I’ll sanity check it with that change to be certain.
>> Yeah, it should. Shame on me and Erik for not catching it. ;-)
>> 
>> No need to respin.
>> 
>> /Magnus
>> 
>>> Best regards,
>>> Robin
>>> 
>>>> /Magnus
>>>> 
>>>>> Webrev (incremental): 
>>>>> http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03-04/
>>>>> 
>>>>> Best regards,
>>>>> Robin
>>>>> 
>>>>>> Otherwise I think this is good now.
>>>>>> 
>>>>>> /Erik
>>>>>>>>> The AWT constructs also confuses me. Maybe you can expand a bit on 
>>>>>>>>> the comment, because it really is non-trivial. You are executing a cp 
>>>>>>>>> for each tmpfile you find? But what if you find multiple tmpfiles? 
>>>>>>>>> There could certainly be multiple commands using @ constructs?
>>>>>>> What it does it actually to invoke fixpath on everything inside the 
>>>>>>> final compile_commands.json file, but in order to make fixpath do that, 
>>>>>>> it presents the compile_commands.json file as an @-argument. Fixpath 
>>>>>>> will then translate that argument to a temporary filename containing 
>>>>>>> corrected paths, and that’s the file we save (since it’s deleted when 
>>>>>>> the invoked command terminates). I’ve updated the comment a bit, 
>>>>>>> hopefully it’s more clear now.
>>>>>>> 
>>>>>>> Another option would be to extend the fixpath utility itself to support 
>>>>>>> some additional argument to just do inline path correction on a given 
>>>>>>> file, but I felt that it would be a more risky change.
>>>>>>> 
>>>>>>>>> * In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r 
>>>>>>>>> build*.log* compile_commands.json)" on a single line.
>>>>>>> Fixed.
>>>>>>> 
>>>>>>>>> * In make/common/MakeBase.gmk: I'd prefer if these functions were 
>>>>>>>>> move to make/common/JdkNativeCompilation.gmk, close to the 
>>>>>>>>> FindSrcDirsForLib etc definitions.
>>>>>>> Fixed.
>>>>>>> 
>>>>>>>>> * In make/common/NativeCompilation.gmk, I'm not entirely happy with 
>>>>>>>>> the $1_OBJ_COMMON_OPTIONS_PRE/POST construct. I understand what you 
>>>>>>>>> are trying to achieve, but I think this gets very hard to read. I 
>>>>>>>>> don't have a perfect solution in mind. But perhaps something like 
>>>>>>>>> this:
>>>>>>>>> $1_DEPS_OPTIONS := $$($1_DEP_FLAG) $$($1_DEP).tmp
>

Re: RFR: 8210459: Add support for generating compile_commands.json

[Robin Westberg]

 * In make/lib/Lib-jdk.accessibility.gmk, this seems double incorrect. 
>>>>>> You are missing a libjawt path segment. And you wanted to use 
>>>>>> FindStaticLib.
>>>> Ah, good catch, I suspect it still works as the jawt.lib file in the 
>>>> modules_lib folder is dependent on jawt.lib in the native folder. Fixed.
>>>> 
>>>>>> Overall, I believe we're misusing the "static lib" concept on Windows. 
>>>>>> The .lib files are not static libs, the way we mean it on unixes. 
>>>>>> Instead, the lib files is some kind of metadata on dll that are used 
>>>>>> when linking with those dlls. So we should introduce a better concept 
>>>>>> for these, and maybe something like FindLibDescriptor (or whatever). 
>>>>>> That should not really be part of this fix, though, so for the moment 
>>>>>> I'm going to accept that we call these "static libs" on Windows.
>>>>>> 
>>>>>> This also makes me wonder how much testing this patch has recieved? I 
>>>>>> know a broken dependency in the worst case only shows up as a race, but 
>>>>>> have you verified it thoroughly even on Windows? And even without 
>>>>>> compile_commands?
>>>> What I’ve been doing, apart from making sure tier1 tests and tier5-builds 
>>>> for all platforms still work, is to “diff” the .cmdline files from a build 
>>>> of “make jdk” with and without the patch applied (and with 
>>>> --with-version-opt= set during configure to avoid the timestamp). The only 
>>>> difference so far is that the EXTRA_OBJECT_FILES change for the 
>>>> make/launcher/Launcher-jdk.pack.gmk moves the files from the command line 
>>>> to an @file, but the contents are the same. (Had to apply a bit of sed to 
>>>> perform this verification after reordering the dependency flags, but still 
>>>> looks correct).
>>>> 
>>>> This obviously won’t catch any subtle dependency mistakes though, not sure 
>>>> if there’s much to be done about that though unfortunately.
>>>> 
>>>> Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03/
>>>> Webrev (incremental): 
>>>> http://cr.openjdk.java.net/~rwestberg/8210459/webrev.02-03/
>>>> 
>>>> Best regards,
>>>> Robin
>>>> 
>>>>>> /Magnus
>>>>>> 
>>>>>>>> Otherwise this looks good now.
>>>>>>> Thanks, I’ll include the latest webrevs with a comment added:
>>>>>>> 
>>>>>>> Incremental: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01-02/
>>>>>>> Full: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.02/
>>>>>>> 
>>>>>>> Best regards,
>>>>>>> Robin
>>>>>>> 
>>>>>>>> /Erik
>>>>>>>>> Webrev (incremental): 
>>>>>>>>> http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00-01/
>>>>>>>>> Webrev (full): 
>>>>>>>>> http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01/
>>>>>>>>> Testing: tier1, builds-tier5
>>>>>>>>> 
>>>>>>>>> Best regards,
>>>>>>>>> Robin
>>>>>>>>> 
>>>>>>>>>> /Erik
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>> On 2018-09-10 06:36, Robin Westberg wrote:
>>>>>>>>>>> Hi all,
>>>>>>>>>>> 
>>>>>>>>>>> Please review the following change that adds support for generating 
>>>>>>>>>>> compile_commands.json as a top-level make target. This is a popular 
>>>>>>>>>>> format for describing how to compile object files for a project, 
>>>>>>>>>>> and is defined in [1]. This file can be used directly by IDEs such 
>>>>>>>>>>> as Visual Studio Code and CLion as well as "language servers" such 
>>>>>>>>>>> as cquery [2] and rtags [3].
>>>>>>>>>>> 
>>>>>>>>>>> While it’s currently possible to generate this file as part of a 
>>>>>>>>>>> full build (using tools such as Bear [4], or simply parsing 
>>>>>>>>>>> .cmdline files), this change aims to make it possible to generate 
>>>>>>>>>>> the compile commands data without actually compiling the object 
>>>>>>>>>>> files. This means that it’s for example possible to start using an 
>>>>>>>>>>> IDE fairly quickly on freshly cloned source, instead of having to 
>>>>>>>>>>> wait for a full build to complete.
>>>>>>>>>>> 
>>>>>>>>>>> This was originally inspired by the work done in [5] by Mikael 
>>>>>>>>>>> Gerdin and Erik Joelsson, but has been through a few revisions 
>>>>>>>>>>> since then. Erik has also provided a lot of assistance with the 
>>>>>>>>>>> current version, thanks Erik!
>>>>>>>>>>> 
>>>>>>>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8210459
>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00/
>>>>>>>>>>> Testing: tier1, builds-tier5
>>>>>>>>>>> 
>>>>>>>>>>> Best regards,
>>>>>>>>>>> Robin
>>>>>>>>>>> 
>>>>>>>>>>> [1] https://clang.llvm.org/docs/JSONCompilationDatabase.html
>>>>>>>>>>> [2] https://github.com/cquery-project/cquery
>>>>>>>>>>> [3] https://github.com/Andersbakken/rtags
>>>>>>>>>>> [4] https://github.com/rizsotto/Bear
>>>>>>>>>>> [5] 
>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-March/026354.html

Re: RFR: 8210459: Add support for generating compile_commands.json

[Robin Westberg]

Hi Erik,

> On 3 Oct 2018, at 20:51, Erik Joelsson  wrote:
> 
> Hello Robin,
> 
> make/CompileCommands.gmk: typo in comment: “prepened"

Fixed. (Couldn’t resist a slight rewording so the text has reflowed a bit, 
sorry!)

> On 2018-10-03 11:09, Robin Westberg wrote:
>> Hi Magnus,
>> 
>> Thanks for reviewing, sorry for taking a while to respond!
>> 
>>> On 19 Sep 2018, at 13:02, Magnus Ihse Bursie 
>>>  wrote:
>>> 
>>> 19 sep. 2018 kl. 09:51 skrev Magnus Ihse Bursie 
>>> :
>>> 
>>>>>>>> In Awt2dLibraries.gmk, I think lines 733 and 735 needs to be fixed as 
>>>>>>>> well. Using FindLib is generally a good idea for this. I suspect there 
>>>>>>>> may be more such instances sprinkled around the makefiles.
>>>>>>> Only fixed the last one, I think the first one is ok as is?
>>>>>> The first one is sort of OK, but it's a questionable construct as the 
>>>>>> $(BUILD_LIBAWT_XAWT) variable may contain additional targets. We used to 
>>>>>> do it that way but these days I prefer the more explicit and precise 
>>>>>> FindLib. In this particular case you get a non needed dependency added 
>>>>>> when running compile-commands with ENABLE_HEADLESS_ONLY=true, which will 
>>>>>> not really affect anything so it doesn't really matter.
>>>>> Yeah I certainly agree, but a quick grep shows that there’s about 50 such 
>>>>> constructs present right now. I wouldn’t mind cleaning those up, but 
>>>>> perhaps that should be in a separate change?
>>>> If that does not affect your patch, you do not need to clean up those 
>>>> constructs.
>>>> 
>>>> I've now looked through your patch. Overall, it looks good. Some minor 
>>>> comments:
>>>> 
>>>> * In  make/CompileCommands.gmk, are you sure the find -exec + construct 
>>>> does not exceed command line limits on problematic platforms such as 
>>>> Solaris and Windows?
>> It runs successfully on Windows and Solaris right now at least, and the 
>> filenames only include a relative path, so should not be sensitive to 
>> working directory location. But I have to admit I’m not sure how close to 
>> the limit it is right now.. Perhaps I can build something around “xargs -s” 
>> instead if you think this is risky?
> I think it would be good to make sure the file list is split using xargs to 
> avoid weird failures in the future. I also just realized it would be good to 
> sort the file list to guarantee stable output.

Makes sense, changed it to use sort and xargs.

Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.04/
Webrev (incremental): 
http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03-04/

Best regards,
Robin

> 
> Otherwise I think this is good now.
> 
> /Erik
>>>> The AWT constructs also confuses me. Maybe you can expand a bit on the 
>>>> comment, because it really is non-trivial. You are executing a cp for each 
>>>> tmpfile you find? But what if you find multiple tmpfiles? There could 
>>>> certainly be multiple commands using @ constructs?
>> What it does it actually to invoke fixpath on everything inside the final 
>> compile_commands.json file, but in order to make fixpath do that, it 
>> presents the compile_commands.json file as an @-argument. Fixpath will then 
>> translate that argument to a temporary filename containing corrected paths, 
>> and that’s the file we save (since it’s deleted when the invoked command 
>> terminates). I’ve updated the comment a bit, hopefully it’s more clear now.
>> 
>> Another option would be to extend the fixpath utility itself to support some 
>> additional argument to just do inline path correction on a given file, but I 
>> felt that it would be a more risky change.
>> 
>>>> * In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r 
>>>> build*.log* compile_commands.json)" on a single line.
>> Fixed.
>> 
>>>> * In make/common/MakeBase.gmk: I'd prefer if these functions were move to 
>>>> make/common/JdkNativeCompilation.gmk, close to the FindSrcDirsForLib etc 
>>>> definitions.
>> Fixed.
>> 
>>>> * In make/common/NativeCompilation.gmk, I'm not entirely happy with the 
>>>> $1_OBJ_COMMON_OPTIONS_PRE/POST construct. I understand what you are trying 
>>>> to achieve, but I think this gets very hard to read. I don't have a 
>>>> perfect sol

Re: RFR: 8210459: Add support for generating compile_commands.json

[Robin Westberg]

catch, I suspect it still works as the jawt.lib file in the 
modules_lib folder is dependent on jawt.lib in the native folder. Fixed.

>> Overall, I believe we're misusing the "static lib" concept on Windows. The 
>> .lib files are not static libs, the way we mean it on unixes. Instead, the 
>> lib files is some kind of metadata on dll that are used when linking with 
>> those dlls. So we should introduce a better concept for these, and maybe 
>> something like FindLibDescriptor (or whatever). That should not really be 
>> part of this fix, though, so for the moment I'm going to accept that we call 
>> these "static libs" on Windows.
>> 
>> This also makes me wonder how much testing this patch has recieved? I know a 
>> broken dependency in the worst case only shows up as a race, but have you 
>> verified it thoroughly even on Windows? And even without compile_commands?

What I’ve been doing, apart from making sure tier1 tests and tier5-builds for 
all platforms still work, is to “diff” the .cmdline files from a build of “make 
jdk” with and without the patch applied (and with --with-version-opt= set 
during configure to avoid the timestamp). The only difference so far is that 
the EXTRA_OBJECT_FILES change for the make/launcher/Launcher-jdk.pack.gmk moves 
the files from the command line to an @file, but the contents are the same. 
(Had to apply a bit of sed to perform this verification after reordering the 
dependency flags, but still looks correct).

This obviously won’t catch any subtle dependency mistakes though, not sure if 
there’s much to be done about that though unfortunately.

Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03/
Webrev (incremental): 
http://cr.openjdk.java.net/~rwestberg/8210459/webrev.02-03/

Best regards,
Robin

>> 
>> /Magnus
>> 
>>> 
>>>> Otherwise this looks good now.
>>> Thanks, I’ll include the latest webrevs with a comment added:
>>> 
>>> Incremental: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01-02/
>>> Full: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.02/
>>> 
>>> Best regards,
>>> Robin
>>> 
>>>> /Erik
>>>>> Webrev (incremental): 
>>>>> http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00-01/
>>>>> Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01/
>>>>> Testing: tier1, builds-tier5
>>>>> 
>>>>> Best regards,
>>>>> Robin
>>>>> 
>>>>>> /Erik
>>>>>> 
>>>>>> 
>>>>>> On 2018-09-10 06:36, Robin Westberg wrote:
>>>>>>> Hi all,
>>>>>>> 
>>>>>>> Please review the following change that adds support for generating 
>>>>>>> compile_commands.json as a top-level make target. This is a popular 
>>>>>>> format for describing how to compile object files for a project, and is 
>>>>>>> defined in [1]. This file can be used directly by IDEs such as Visual 
>>>>>>> Studio Code and CLion as well as "language servers" such as cquery [2] 
>>>>>>> and rtags [3].
>>>>>>> 
>>>>>>> While it’s currently possible to generate this file as part of a full 
>>>>>>> build (using tools such as Bear [4], or simply parsing .cmdline files), 
>>>>>>> this change aims to make it possible to generate the compile commands 
>>>>>>> data without actually compiling the object files. This means that it’s 
>>>>>>> for example possible to start using an IDE fairly quickly on freshly 
>>>>>>> cloned source, instead of having to wait for a full build to complete.
>>>>>>> 
>>>>>>> This was originally inspired by the work done in [5] by Mikael Gerdin 
>>>>>>> and Erik Joelsson, but has been through a few revisions since then. 
>>>>>>> Erik has also provided a lot of assistance with the current version, 
>>>>>>> thanks Erik!
>>>>>>> 
>>>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8210459
>>>>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00/
>>>>>>> Testing: tier1, builds-tier5
>>>>>>> 
>>>>>>> Best regards,
>>>>>>> Robin
>>>>>>> 
>>>>>>> [1] https://clang.llvm.org/docs/JSONCompilationDatabase.html
>>>>>>> [2] https://github.com/cquery-project/cquery
>>>>>>> [3] https://github.com/Andersbakken/rtags
>>>>>>> [4] https://github.com/rizsotto/Bear
>>>>>>> [5] 
>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-March/026354.html

Re: RFR: 8210459: Add support for generating compile_commands.json

[Robin Westberg]

Hi Erik,

> On 14 Sep 2018, at 19:21, Erik Joelsson  wrote:
> 
>>> Main.gmk:888: I think you meant to add test-compile-commands here, which 
>>> also requires you to define that target.
>> Fixed, the tests are now invoked from a top-level target. I also added a 
>> second test to ensure that the no-build-libraries property of this actually 
>> stays intact.
> This is ok, but the test will fail unless run from a clean output dir. This 
> should probably be noted in a comment at least.

Yes, good point, I tried briefly to include a clean as a prerequisite target to 
the test, but it refused to execute sequentially. But perhaps it’s good enough 
with a comment, added one.

>>> In Awt2dLibraries.gmk, I think lines 733 and 735 needs to be fixed as well. 
>>> Using FindLib is generally a good idea for this. I suspect there may be 
>>> more such instances sprinkled around the makefiles.
>> Only fixed the last one, I think the first one is ok as is?
> The first one is sort of OK, but it's a questionable construct as the 
> $(BUILD_LIBAWT_XAWT) variable may contain additional targets. We used to do 
> it that way but these days I prefer the more explicit and precise FindLib. In 
> this particular case you get a non needed dependency added when running 
> compile-commands with ENABLE_HEADLESS_ONLY=true, which will not really affect 
> anything so it doesn't really matter.

Yeah I certainly agree, but a quick grep shows that there’s about 50 such 
constructs present right now. I wouldn’t mind cleaning those up, but perhaps 
that should be in a separate change?

> Otherwise this looks good now.

Thanks, I’ll include the latest webrevs with a comment added:

Incremental: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01-02/
Full: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.02/

Best regards,
Robin

> 
> /Erik
>> Webrev (incremental): 
>> http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00-01/
>> Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01/
>> Testing: tier1, builds-tier5
>> 
>> Best regards,
>> Robin
>> 
>>> /Erik
>>> 
>>> 
>>> On 2018-09-10 06:36, Robin Westberg wrote:
>>>> Hi all,
>>>> 
>>>> Please review the following change that adds support for generating 
>>>> compile_commands.json as a top-level make target. This is a popular format 
>>>> for describing how to compile object files for a project, and is defined 
>>>> in [1]. This file can be used directly by IDEs such as Visual Studio Code 
>>>> and CLion as well as "language servers" such as cquery [2] and rtags [3].
>>>> 
>>>> While it’s currently possible to generate this file as part of a full 
>>>> build (using tools such as Bear [4], or simply parsing .cmdline files), 
>>>> this change aims to make it possible to generate the compile commands data 
>>>> without actually compiling the object files. This means that it’s for 
>>>> example possible to start using an IDE fairly quickly on freshly cloned 
>>>> source, instead of having to wait for a full build to complete.
>>>> 
>>>> This was originally inspired by the work done in [5] by Mikael Gerdin and 
>>>> Erik Joelsson, but has been through a few revisions since then. Erik has 
>>>> also provided a lot of assistance with the current version, thanks Erik!
>>>> 
>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8210459
>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00/
>>>> Testing: tier1, builds-tier5
>>>> 
>>>> Best regards,
>>>> Robin
>>>> 
>>>> [1] https://clang.llvm.org/docs/JSONCompilationDatabase.html
>>>> [2] https://github.com/cquery-project/cquery
>>>> [3] https://github.com/Andersbakken/rtags
>>>> [4] https://github.com/rizsotto/Bear
>>>> [5] 
>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-March/026354.html

Re: RFR: 8210459: Add support for generating compile_commands.json

[Robin Westberg]

Hi Erik,

Thanks for the review!

> On 10 Sep 2018, at 19:09, Erik Joelsson  wrote:
> 
> Hello Robin,
> 
> Overall very nice work. A few things though:
> 
> The output dir compile_commands should use a dash instead to fit with current 
> patterns. That way the clean target will also be all dashes.

Fixed. I actually removed the clean target, as the output is located in a 
subfolder of make-support, so probably don’t need a special one.

> Main.gmk:888: I think you meant to add test-compile-commands here, which also 
> requires you to define that target.

Fixed, the tests are now invoked from a top-level target. I also added a second 
test to ensure that the no-build-libraries property of this actually stays 
intact.

> In NativeCompilation.gmk:739: I don't see a reason for = assignment, so 
> please use :=. On the next line, please indent for continuation with 4 spaces.

Fixed.

> In Launcher-jdk.pack.gmk, this SetupJdkExecutable can be rewritten to use the 
> EXTRA_OBJECT_FILES parameter (inherited from SetupNativeCompilation macro) 
> for UNPACKEXE_ZIPOBJS instead of adding them to LDFLAGS. Then the explicit 
> dependency declaration can go away completely.

Fixed.

> In Awt2dLibraries.gmk, I think lines 733 and 735 needs to be fixed as well. 
> Using FindLib is generally a good idea for this. I suspect there may be more 
> such instances sprinkled around the makefiles.

Only fixed the last one, I think the first one is ok as is?

Webrev (incremental): 
http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00-01/
Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01/
Testing: tier1, builds-tier5

Best regards,
Robin

> 
> /Erik
> 
> 
> On 2018-09-10 06:36, Robin Westberg wrote:
>> Hi all,
>> 
>> Please review the following change that adds support for generating 
>> compile_commands.json as a top-level make target. This is a popular format 
>> for describing how to compile object files for a project, and is defined in 
>> [1]. This file can be used directly by IDEs such as Visual Studio Code and 
>> CLion as well as "language servers" such as cquery [2] and rtags [3].
>> 
>> While it’s currently possible to generate this file as part of a full build 
>> (using tools such as Bear [4], or simply parsing .cmdline files), this 
>> change aims to make it possible to generate the compile commands data 
>> without actually compiling the object files. This means that it’s for 
>> example possible to start using an IDE fairly quickly on freshly cloned 
>> source, instead of having to wait for a full build to complete.
>> 
>> This was originally inspired by the work done in [5] by Mikael Gerdin and 
>> Erik Joelsson, but has been through a few revisions since then. Erik has 
>> also provided a lot of assistance with the current version, thanks Erik!
>> 
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8210459
>> Webrev: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00/
>> Testing: tier1, builds-tier5
>> 
>> Best regards,
>> Robin
>> 
>> [1] https://clang.llvm.org/docs/JSONCompilationDatabase.html
>> [2] https://github.com/cquery-project/cquery
>> [3] https://github.com/Andersbakken/rtags
>> [4] https://github.com/rizsotto/Bear
>> [5] http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-March/026354.html
> 

RFR: 8210459: Add support for generating compile_commands.json

[Robin Westberg]

Hi all,

Please review the following change that adds support for generating 
compile_commands.json as a top-level make target. This is a popular format for 
describing how to compile object files for a project, and is defined in [1]. 
This file can be used directly by IDEs such as Visual Studio Code and CLion as 
well as "language servers" such as cquery [2] and rtags [3]. 

While it’s currently possible to generate this file as part of a full build 
(using tools such as Bear [4], or simply parsing .cmdline files), this change 
aims to make it possible to generate the compile commands data without actually 
compiling the object files. This means that it’s for example possible to start 
using an IDE fairly quickly on freshly cloned source, instead of having to wait 
for a full build to complete.

This was originally inspired by the work done in [5] by Mikael Gerdin and Erik 
Joelsson, but has been through a few revisions since then. Erik has also 
provided a lot of assistance with the current version, thanks Erik!

Issue: https://bugs.openjdk.java.net/browse/JDK-8210459
Webrev: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00/
Testing: tier1, builds-tier5

Best regards,
Robin

[1] https://clang.llvm.org/docs/JSONCompilationDatabase.html
[2] https://github.com/cquery-project/cquery
[3] https://github.com/Andersbakken/rtags
[4] https://github.com/rizsotto/Bear
[5] http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-March/026354.html

Re: RFR: 8199619: Building HotSpot on Windows should define NOMINMAX

[Robin Westberg]

Thanks Erik!

Best regards,
Robin

> On 28 Mar 2018, at 17:47, Erik Joelsson <erik.joels...@oracle.com> wrote:
> 
> I will sponsor the change.
> 
> /Erik
> 
> 
> On 2018-03-28 06:43, Robin Westberg wrote:
>> Hi Kim,
>> 
>>> On 26 Mar 2018, at 18:34, Kim Barrett <kim.barr...@oracle.com> wrote:
>>> 
>>>> On Mar 26, 2018, at 11:01 AM, Robin Westberg <robin.westb...@oracle.com> 
>>>> wrote:
>>>> 
>>>> Hi all,
>>>> 
>>>> Please review this small change that defines the NOMINMAX macro when 
>>>> building HotSpot on Windows.
>>>> 
>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8199619
>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8199619/webrev.00/ 
>>>> <http://cr.openjdk.java.net/~rwestberg/8199619/webrev.00/>
>>>> Testing: building with/without precompiled headers, hs-tier1
>>>> 
>>>> Best regards,
>>>> Robin
>>> Looks good.
>> Thanks for reviewing!
>> 
>>> This change will have a (easy to resolve) merge conflict with your fix for 
>>> JDK-8199736, right?
>> Indeed, the flag definitions should go on a single line I think. I’ll try to 
>> get this one in first and rebase 8199736 afterwards.
>> 
>> So, if anyone would be willing to sponsor this change, here’s an updated 
>> webrev with a proper mercurial changeset (no other changes):
>> http://cr.openjdk.java.net/~rwestberg/8199619/webrev.01/ 
>> <http://cr.openjdk.java.net/~rwestberg/8199619/webrev.01/>
>> 
>> Best regards,
>> Robin
>> 
> 

Re: RFR: 8199619: Building HotSpot on Windows should define NOMINMAX

[Robin Westberg]

Hi Kim,

> On 26 Mar 2018, at 18:34, Kim Barrett <kim.barr...@oracle.com> wrote:
> 
>> On Mar 26, 2018, at 11:01 AM, Robin Westberg <robin.westb...@oracle.com> 
>> wrote:
>> 
>> Hi all,
>> 
>> Please review this small change that defines the NOMINMAX macro when 
>> building HotSpot on Windows.
>> 
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8199619
>> Webrev: http://cr.openjdk.java.net/~rwestberg/8199619/webrev.00/ 
>> <http://cr.openjdk.java.net/~rwestberg/8199619/webrev.00/>
>> Testing: building with/without precompiled headers, hs-tier1
>> 
>> Best regards,
>> Robin
> 
> Looks good.

Thanks for reviewing! 

> This change will have a (easy to resolve) merge conflict with your fix for 
> JDK-8199736, right?

Indeed, the flag definitions should go on a single line I think. I’ll try to 
get this one in first and rebase 8199736 afterwards.

So, if anyone would be willing to sponsor this change, here’s an updated webrev 
with a proper mercurial changeset (no other changes):
http://cr.openjdk.java.net/~rwestberg/8199619/webrev.01/ 
<http://cr.openjdk.java.net/~rwestberg/8199619/webrev.01/>

Best regards,
Robin

Re: RFR: 8199736: Define WIN32_LEAN_AND_MEAN before including windows.h

[Robin Westberg]

Hi David,

Thanks for reviewing! I’ll delay progressing this one a bit until 8199619 is 
integrated.

Best regards,
Robin

> On 27 Mar 2018, at 02:57, David Holmes <david.hol...@oracle.com> wrote:
> 
> Looks good to me.
> 
> Thanks,
> David
> 
> On 27/03/2018 1:01 AM, Robin Westberg wrote:
>> Hi David,
>> Thanks for taking a look!
>>> On 26 Mar 2018, at 01:03, David Holmes <david.hol...@oracle.com 
>>> <mailto:david.hol...@oracle.com>> wrote:
>>> 
>>> Hi Robin,
>>> 
>>> On 23/03/2018 10:37 PM, Robin Westberg wrote:
>>>> Hi Kim & Erik,
>>>> Certainly makes sense to define it from the build system, I’ve updated the 
>>>> patch accordingly:
>>>> Full: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01/ 
>>>> <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01/>
>>>> Incremental: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00-01/ 
>>>> <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00-01/>
>>> 
>>> I'm a little unclear on the hotspot changes. If we define 
>>> WIN32_LEAN_AND_MEAN then certain APIs like sockets are excluded from 
>>> windows.h so we then have to include the specific header files like 
>>> winsock2.h - is that right?
>> Yep that’s correct, headers like winsock, dde, ole, shellapi and a few other 
>> uncommon ones are no longer included from windows.h when this is defined.
>>> src/hotspot/share/interpreter/bytecodes.cpp
>>> 
>>> I'm curious about this change. u_short comes from types.h on non-Windows, 
>>> is it simply missing on Windows (at least once we have WIN32_LEAN_AND_MEAN 
>>> defined) ?
>> Yeah, on Windows these comes from winsock(2).h:
>> /*
>>  * Basic system type definitions, taken from the BSD file sys/types.h.
>>  */
>> typedef unsigned char   u_char;
>> typedef unsigned short  u_short;
>> typedef unsigned intu_int;
>> typedef unsigned long   u_long;
>> I noticed that one of these (u_char) is also defined in 
>> globalDefinitions.hpp so could perhaps define u_short there, or include 
>> winsock2.h globally again. But since it was only used in a single place in 
>> the existing code it seemed simple enough to just expand the typedef there.
>>> src/hotspot/share/utilities/ostream.cpp
>>> 
>>> 1029 #endif
>>> 1030 #if defined(_WINDOWS)
>>> 
>>> Using elif could be marginally faster given the two sets of conditions are 
>>> mutually exclusive.
>> Good point, will change it.
>> I also had to move the flag definition to adapt to the latest changes in the 
>> hs repo, cc’ing build-dev again to make sure I got it right.
>> Updated webrev (full): 
>> http://cr.openjdk.java.net/~rwestberg/8199736/webrev.02/
>> Updated webrev (incremental): 
>> http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01-02/
>> Best regards,
>> Robin
>>> 
>>> Thanks,
>>> David
>>> 
>>>> (Not quite sure if the definition belongs where I put it or a bit later 
>>>> where most other windows-specific JVM flags are defined, but seemed 
>>>> reasonable to put it close to where it is defined for the JDK libraries).
>>>> Best regards,
>>>> Robin
>>>>> On 22 Mar 2018, at 16:52, Kim Barrett <kim.barr...@oracle.com 
>>>>> <mailto:kim.barr...@oracle.com>> wrote:
>>>>> 
>>>>>> On Mar 22, 2018, at 10:34 AM, Robin Westberg <robin.westb...@oracle.com 
>>>>>> <mailto:robin.westb...@oracle.com>> wrote:
>>>>>> 
>>>>>> Hi all,
>>>>>> 
>>>>>> Please review the following change that defines WIN32_LEAN_AND_MEAN [1] 
>>>>>> before including windows.h. This marginally improves build times, and 
>>>>>> makes it possible to include winsock2.h.
>>>>>> 
>>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8199736 
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8199736>
>>>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/ 
>>>>>> <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/>
>>>>>> Testing: hs-tier1
>>>>>> 
>>>>>> Best regards,
>>>>>> Robin
>>>>>> 
>>>>>> [1] 
>>>>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745%28v=vs.85%29.aspx#faster_builds_with_smaller_header_files
>>>>>>  
>>>>>> <https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745(v=vs.85).aspx#faster_builds_with_smaller_header_files>
>>>>> 
>>>>> I think the addition of the WIN32_LEAN_AND_MEAN definition should be done 
>>>>> through the build
>>>>> system, so that it applies everywhere.
>>>>> 

Re: RFR: 8199619: Building HotSpot on Windows should define NOMINMAX

[Robin Westberg]

Hi Magnus,

Thanks for the review!

Best regards,
Robin

> On 26 Mar 2018, at 23:24, Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com> 
> wrote:
> 
> On 2018-03-26 17:01, Robin Westberg wrote:
>> Hi all,
>> 
>> Please review this small change that defines the NOMINMAX macro when 
>> building HotSpot on Windows.
>> 
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8199619
>> Webrev: http://cr.openjdk.java.net/~rwestberg/8199619/webrev.00/ 
>> <http://cr.openjdk.java.net/~rwestberg/8199619/webrev.00/>
> Looks good to me.
> 
> /Magnus
> 
>> Testing: building with/without precompiled headers, hs-tier1
>> 
>> Best regards,
>> Robin
> 

Re: RFR: 8199619: Building HotSpot on Windows should define NOMINMAX

[Robin Westberg]

Hi Erik,

Thanks for reviewing!

Best regards,
Robin

> On 26 Mar 2018, at 17:50, Erik Joelsson <erik.joels...@oracle.com> wrote:
> 
> Looks good.
> 
> /Erik
> 
> 
> On 2018-03-26 08:01, Robin Westberg wrote:
>> Hi all,
>> 
>> Please review this small change that defines the NOMINMAX macro when 
>> building HotSpot on Windows.
>> 
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8199619
>> Webrev: http://cr.openjdk.java.net/~rwestberg/8199619/webrev.00/ 
>> <http://cr.openjdk.java.net/~rwestberg/8199619/webrev.00/>
>> Testing: building with/without precompiled headers, hs-tier1
>> 
>> Best regards,
>> Robin
> 

RFR: 8199619: Building HotSpot on Windows should define NOMINMAX

[Robin Westberg]

Hi all,

Please review this small change that defines the NOMINMAX macro when building 
HotSpot on Windows.

Issue: https://bugs.openjdk.java.net/browse/JDK-8199619
Webrev: http://cr.openjdk.java.net/~rwestberg/8199619/webrev.00/ 

Testing: building with/without precompiled headers, hs-tier1

Best regards,
Robin

Re: RFR: 8199736: Define WIN32_LEAN_AND_MEAN before including windows.h

[Robin Westberg]

Hi David,

Thanks for taking a look!

> On 26 Mar 2018, at 01:03, David Holmes <david.hol...@oracle.com> wrote:
> 
> Hi Robin,
> 
> On 23/03/2018 10:37 PM, Robin Westberg wrote:
>> Hi Kim & Erik,
>> Certainly makes sense to define it from the build system, I’ve updated the 
>> patch accordingly:
>> Full: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01/ 
>> <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01/>
>> Incremental: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00-01/ 
>> <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00-01/>
> 
> I'm a little unclear on the hotspot changes. If we define WIN32_LEAN_AND_MEAN 
> then certain APIs like sockets are excluded from windows.h so we then have to 
> include the specific header files like winsock2.h - is that right?

Yep that’s correct, headers like winsock, dde, ole, shellapi and a few other 
uncommon ones are no longer included from windows.h when this is defined.

> src/hotspot/share/interpreter/bytecodes.cpp
> 
> I'm curious about this change. u_short comes from types.h on non-Windows, is 
> it simply missing on Windows (at least once we have WIN32_LEAN_AND_MEAN 
> defined) ?

Yeah, on Windows these comes from winsock(2).h:

/*
 * Basic system type definitions, taken from the BSD file sys/types.h.
 */
typedef unsigned char   u_char;
typedef unsigned short  u_short;
typedef unsigned intu_int;
typedef unsigned long   u_long;

I noticed that one of these (u_char) is also defined in globalDefinitions.hpp 
so could perhaps define u_short there, or include winsock2.h globally again. 
But since it was only used in a single place in the existing code it seemed 
simple enough to just expand the typedef there.
 
> src/hotspot/share/utilities/ostream.cpp
> 
> 1029 #endif
> 1030 #if defined(_WINDOWS)
> 
> Using elif could be marginally faster given the two sets of conditions are 
> mutually exclusive.

Good point, will change it.

I also had to move the flag definition to adapt to the latest changes in the hs 
repo, cc’ing build-dev again to make sure I got it right.

Updated webrev (full): http://cr.openjdk.java.net/~rwestberg/8199736/webrev.02/ 
<http://cr.openjdk.java.net/~rwestberg/8199736/webrev.02/>
Updated webrev (incremental): 
http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01-02/ 
<http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01-02/>

Best regards,
Robin

> 
> Thanks,
> David
> 
>> (Not quite sure if the definition belongs where I put it or a bit later 
>> where most other windows-specific JVM flags are defined, but seemed 
>> reasonable to put it close to where it is defined for the JDK libraries).
>> Best regards,
>> Robin
>>> On 22 Mar 2018, at 16:52, Kim Barrett <kim.barr...@oracle.com> wrote:
>>> 
>>>> On Mar 22, 2018, at 10:34 AM, Robin Westberg <robin.westb...@oracle.com> 
>>>> wrote:
>>>> 
>>>> Hi all,
>>>> 
>>>> Please review the following change that defines WIN32_LEAN_AND_MEAN [1] 
>>>> before including windows.h. This marginally improves build times, and 
>>>> makes it possible to include winsock2.h.
>>>> 
>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8199736 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8199736>
>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/ 
>>>> <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/>
>>>> Testing: hs-tier1
>>>> 
>>>> Best regards,
>>>> Robin
>>>> 
>>>> [1] 
>>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745%28v=vs.85%29.aspx#faster_builds_with_smaller_header_files
>>>>  
>>>> <https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745(v=vs.85).aspx#faster_builds_with_smaller_header_files>
>>> 
>>> I think the addition of the WIN32_LEAN_AND_MEAN definition should be done 
>>> through the build
>>> system, so that it applies everywhere.
>>> 

Re: RFR: 8199736: Define WIN32_LEAN_AND_MEAN before including windows.h

[Robin Westberg]

Hi Magnus,

Thanks for the review!

Best regards,
Robin

> On 23 Mar 2018, at 16:43, Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com> 
> wrote:
> 
> This looks good to me.
> 
> /Magnus
> 
>> 23 mars 2018 kl. 14:58 skrev Erik Joelsson <erik.joels...@oracle.com>:
>> 
>> I think this looks good, but Magnus is currently refactoring the flags 
>> handling in configure so better get his input as well. (adding build-dev)
>> 
>> /Erik
>> 
>> 
>>> On 2018-03-23 05:37, Robin Westberg wrote:
>>> Hi Kim & Erik,
>>> 
>>> Certainly makes sense to define it from the build system, I’ve updated the 
>>> patch accordingly:
>>> 
>>> Full: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01/ 
>>> <http://cr.openjdk.java.net/%7Erwestberg/8199736/webrev.01/>
>>> Incremental: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00-01/ 
>>> <http://cr.openjdk.java.net/%7Erwestberg/8199736/webrev.00-01/>
>>> 
>>> (Not quite sure if the definition belongs where I put it or a bit later 
>>> where most other windows-specific JVM flags are defined, but seemed 
>>> reasonable to put it close to where it is defined for the JDK libraries).
>>> 
>>> Best regards,
>>> Robin
>>> 
>>>>>> On 22 Mar 2018, at 16:52, Kim Barrett <kim.barr...@oracle.com 
>>>>>> <mailto:kim.barr...@oracle.com>> wrote:
>>>>> 
>>>>> On Mar 22, 2018, at 10:34 AM, Robin Westberg <robin.westb...@oracle.com 
>>>>> <mailto:robin.westb...@oracle.com>> wrote:
>>>>> 
>>>>> Hi all,
>>>>> 
>>>>> Please review the following change that defines WIN32_LEAN_AND_MEAN [1] 
>>>>> before including windows.h. This marginally improves build times, and 
>>>>> makes it possible to include winsock2.h.
>>>>> 
>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8199736 
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8199736>
>>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/ 
>>>>> <http://cr.openjdk.java.net/%7Erwestberg/8199736/webrev.00/> 
>>>>> <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/ 
>>>>> <http://cr.openjdk.java.net/%7Erwestberg/8199736/webrev.00/>>
>>>>> Testing: hs-tier1
>>>>> 
>>>>> Best regards,
>>>>> Robin
>>>>> 
>>>>> [1] 
>>>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745%28v=vs.85%29.aspx#faster_builds_with_smaller_header_files
>>>>>  
>>>>> <https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745(v=vs.85).aspx#faster_builds_with_smaller_header_files
>>>>>  
>>>>> <https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745%28v=vs.85%29.aspx#faster_builds_with_smaller_header_files>>
>>>> 
>>>> I think the addition of the WIN32_LEAN_AND_MEAN definition should be done 
>>>> through the build
>>>> system, so that it applies everywhere.
>>>> 
>>> 
>> 
> 

Re: RFR: 8199736: Define WIN32_LEAN_AND_MEAN before including windows.h

[Robin Westberg]

Hi Erik,

Thanks for reviewing!

Best regards,
Robin

> On 23 Mar 2018, at 14:58, Erik Joelsson <erik.joels...@oracle.com> wrote:
> 
> I think this looks good, but Magnus is currently refactoring the flags 
> handling in configure so better get his input as well. (adding build-dev)
> /Erik
> 
> On 2018-03-23 05:37, Robin Westberg wrote:
>> Hi Kim & Erik,
>> 
>> Certainly makes sense to define it from the build system, I’ve updated the 
>> patch accordingly:
>> 
>> Full: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01/ 
>> <http://cr.openjdk.java.net/%7Erwestberg/8199736/webrev.01/>
>> Incremental: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00-01/ 
>> <http://cr.openjdk.java.net/%7Erwestberg/8199736/webrev.00-01/>
>> 
>> (Not quite sure if the definition belongs where I put it or a bit later 
>> where most other windows-specific JVM flags are defined, but seemed 
>> reasonable to put it close to where it is defined for the JDK libraries).
>> 
>> Best regards,
>> Robin
>> 
>>> On 22 Mar 2018, at 16:52, Kim Barrett <kim.barr...@oracle.com 
>>> <mailto:kim.barr...@oracle.com>> wrote:
>>> 
>>>> On Mar 22, 2018, at 10:34 AM, Robin Westberg <robin.westb...@oracle.com 
>>>> <mailto:robin.westb...@oracle.com>> wrote:
>>>> 
>>>> Hi all,
>>>> 
>>>> Please review the following change that defines WIN32_LEAN_AND_MEAN [1] 
>>>> before including windows.h. This marginally improves build times, and 
>>>> makes it possible to include winsock2.h.
>>>> 
>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8199736 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8199736> 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8199736 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8199736>>
>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/ 
>>>> <http://cr.openjdk.java.net/%7Erwestberg/8199736/webrev.00/> 
>>>> <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/ 
>>>> <http://cr.openjdk.java.net/%7Erwestberg/8199736/webrev.00/>>
>>>> Testing: hs-tier1
>>>> 
>>>> Best regards,
>>>> Robin
>>>> 
>>>> [1] 
>>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745%28v=vs.85%29.aspx#faster_builds_with_smaller_header_files
>>>>  
>>>> <https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745%28v=vs.85%29.aspx#faster_builds_with_smaller_header_files><https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745(v=vs.85).aspx#faster_builds_with_smaller_header_files
>>>>  
>>>> <https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745%28v=vs.85%29.aspx#faster_builds_with_smaller_header_files>>
>>> 
>>> I think the addition of the WIN32_LEAN_AND_MEAN definition should be done 
>>> through the build
>>> system, so that it applies everywhere.
>>> 
>> 
>