Re: RFR: 8287828: Fix so that one can select jtreg test case by ID from make [v5]

2022-06-08 Thread Magnus Ihse Bursie
On Tue, 7 Jun 2022 18:15:30 GMT, Leo Korinth  wrote:

>> One can select a testcase by ID when running a jtreg test case directly from 
>> jtreg (using the testcase.java#testID syntax). However, this has not been 
>> possible to do when launching jtreg indirectly from make.
>> 
>> This fix attempts to address this issue. I have not tested this thoroughly 
>> yet, I wanted to show the code to get feedback first. The idea is to *not* 
>> emulated destructive imperative assignments through the use of eval. I 
>> instead try to handle it in a functional style without reassigning 
>> variables. I have tried to make the change as small as possible.
>> 
>> I am not used to change the build system, so I would appreciate thorough 
>> review.
>> 
>> `IfAppend` and `IfPrepend` are general purpose functions. If similar 
>> functions exists elsewhere inside the code base or in make itself I should 
>> probably use those instead. If they do not exist, they might be useful 
>> elsewhere and maybe should be placed in a common make file instead of the 
>> RunTests.gmk file.
>
> Leo Korinth has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   use test id in comments

I apologize for the late review. The latest version look very good indeed, 
arguably even much better than the original. (Apart from it fixing the bug, 
that is.)

Thank you for contributing to the build system! You're welcome back any time 
you want. ;-)

-

Marked as reviewed by ihse (Reviewer).

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


Re: RFR: 8287828: Fix so that one can select jtreg test case by ID from make [v5]

2022-06-08 Thread Erik Joelsson
On Tue, 7 Jun 2022 18:15:30 GMT, Leo Korinth  wrote:

>> One can select a testcase by ID when running a jtreg test case directly from 
>> jtreg (using the testcase.java#testID syntax). However, this has not been 
>> possible to do when launching jtreg indirectly from make.
>> 
>> This fix attempts to address this issue. I have not tested this thoroughly 
>> yet, I wanted to show the code to get feedback first. The idea is to *not* 
>> emulated destructive imperative assignments through the use of eval. I 
>> instead try to handle it in a functional style without reassigning 
>> variables. I have tried to make the change as small as possible.
>> 
>> I am not used to change the build system, so I would appreciate thorough 
>> review.
>> 
>> `IfAppend` and `IfPrepend` are general purpose functions. If similar 
>> functions exists elsewhere inside the code base or in make itself I should 
>> probably use those instead. If they do not exist, they might be useful 
>> elsewhere and maybe should be placed in a common make file instead of the 
>> RunTests.gmk file.
>
> Leo Korinth has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   use test id in comments

Marked as reviewed by erikj (Reviewer).

-

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


Re: RFR: 8287828: Fix so that one can select jtreg test case by ID from make [v5]

2022-06-07 Thread Leo Korinth
> One can select a testcase by ID when running a jtreg test case directly from 
> jtreg (using the testcase.java#testID syntax). However, this has not been 
> possible to do when launching jtreg indirectly from make.
> 
> This fix attempts to address this issue. I have not tested this thoroughly 
> yet, I wanted to show the code to get feedback first. The idea is to *not* 
> emulated destructive imperative assignments through the use of eval. I 
> instead try to handle it in a functional style without reassigning variables. 
> I have tried to make the change as small as possible.
> 
> I am not used to change the build system, so I would appreciate thorough 
> review.
> 
> `IfAppend` and `IfPrepend` are general purpose functions. If similar 
> functions exists elsewhere inside the code base or in make itself I should 
> probably use those instead. If they do not exist, they might be useful 
> elsewhere and maybe should be placed in a common make file instead of the 
> RunTests.gmk file.

Leo Korinth has updated the pull request incrementally with one additional 
commit since the last revision:

  use test id in comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/9028/files
  - new: https://git.openjdk.java.net/jdk/pull/9028/files/ac9631e2..a6d51c2a

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

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

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