Re: RFR: JDK-8257547: Handle multiple prereqs on the same line in deps files [v2]

2020-12-02 Thread Tim Bell
On Wed, 2 Dec 2020 21:41:26 GMT, Erik Joelsson  wrote:

>> test/make/TestFixDepsFile.gmk line 60:
>> 
>>> 58: $(ECHO) " $(WORKSPACE_ROOT)/bar/baz \\" >> $(DEPS_FILE).expected
>>> 59: $(ECHO) " /foo/baz" >> $(DEPS_FILE).expected
>>> 60: $(DIFF) $(DEPS_FILE).expected $(DEPS_FILE)
>> 
>> Does this need to be:
>> $(DIFF) $(DEPS_FILE).expected $(DEPS_FILE).tmp
>
> No, the fix-deps-file macro takes file.tmp as input and outputs into file, so 
> $(DEPS_FILE) is the output file from the macro in this case.

OK

-

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


Re: RFR: JDK-8257547: Handle multiple prereqs on the same line in deps files [v2]

2020-12-02 Thread Erik Joelsson
On Wed, 2 Dec 2020 20:01:08 GMT, Tim Bell  wrote:

>> Erik Joelsson has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Added test
>
> test/make/TestFixDepsFile.gmk line 60:
> 
>> 58:  $(ECHO) " $(WORKSPACE_ROOT)/bar/baz \\" >> $(DEPS_FILE).expected
>> 59:  $(ECHO) " /foo/baz" >> $(DEPS_FILE).expected
>> 60:  $(DIFF) $(DEPS_FILE).expected $(DEPS_FILE)
> 
> Does this need to be:
> $(DIFF) $(DEPS_FILE).expected $(DEPS_FILE).tmp

No, the fix-deps-file macro takes file.tmp as input and outputs into file, so 
$(DEPS_FILE) is the output file from the macro in this case.

-

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


Re: RFR: JDK-8257547: Handle multiple prereqs on the same line in deps files [v2]

2020-12-02 Thread Tim Bell
On Wed, 2 Dec 2020 17:55:18 GMT, Erik Joelsson  wrote:

>> After fixing JDK-8256810 and starting to look into backporting it, I 
>> discovered more potential failing cases. Certain versions of GCC may 
>> sometimes output multiple prerequisite files on the same line. I think the 
>> easiest way to handle this new issue is to split such lines.
>> 
>> When splitting lines, we need to make sure to not just split on any space. 
>> Some compilers output the : of the rule with a leading space, and already 
>> split lines will have a space before the backslash.
>
> Erik Joelsson has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added test

test/make/TestFixDepsFile.gmk line 60:

> 58:   $(ECHO) " $(WORKSPACE_ROOT)/bar/baz \\" >> $(DEPS_FILE).expected
> 59:   $(ECHO) " /foo/baz" >> $(DEPS_FILE).expected
> 60:   $(DIFF) $(DEPS_FILE).expected $(DEPS_FILE)

Does this need to be:
$(DIFF) $(DEPS_FILE).expected $(DEPS_FILE).tmp

-

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


Re: RFR: JDK-8257547: Handle multiple prereqs on the same line in deps files [v2]

2020-12-02 Thread Erik Joelsson
> After fixing JDK-8256810 and starting to look into backporting it, I 
> discovered more potential failing cases. Certain versions of GCC may 
> sometimes output multiple prerequisite files on the same line. I think the 
> easiest way to handle this new issue is to split such lines.
> 
> When splitting lines, we need to make sure to not just split on any space. 
> Some compilers output the : of the rule with a leading space, and already 
> split lines will have a space before the backslash.

Erik Joelsson has updated the pull request incrementally with one additional 
commit since the last revision:

  Added test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1548/files
  - new: https://git.openjdk.java.net/jdk/pull/1548/files/3164af58..9f8a7edb

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

  Stats: 71 lines in 2 files changed: 70 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1548.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1548/head:pull/1548

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