RFR(XS): 8146425: After change 8142907 'EXCLUDE_FILE' is wrongly interpreted as pattern

Tue Jan 5 17:49:34 UTC 2016

Looks good to me. Thank you Volker!

I will make sure I don't break this with the upcoming changes.

/Erik

On 2016-01-05 18:42, Volker Simonis wrote:
> OK, I only want to get this fixed now.
>
> Do you agree with the new fix which is AIX-only and prepends '/' to
> the filename as suggested by you:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2016/8146425.v1/
>
> Regards,
> Volker
>
> On Tue, Jan 5, 2016 at 5:47 PM, Erik Joelsson <erik.joelsson at oracle.com> wrote:
>> Hello Volker,
>>
>> On 2016-01-05 14:27, Volker Simonis wrote:
>>> I see, but as far as I understand all you are saying only applies to
>>> the new hotspot-build system in build-infra. So I really don't see why
>>> these changes had to be integrated into jdk9-dev. As far as I can see,
>>> EXCLUDE_PATTERNS isn't currently used anywhere in jdk9-dev.
>> That is correct. We use the functionality in build-infra/jdk9 for the new
>> hotspot build. We integrated a bunch of general build changes early to
>> reduce the burden of maintaining those changes in the build-infra
>> repository. This was one of them. Some were relevant to jdk9, this one
>> wasn't yet.
>>> And I'm a little reluctant to use '/NativeThread.c' as you proposed
>>> because according to the link you posted this pattern will be
>>> interpreted as an absolute path with the upcoming changes you are
>>> doing.
>> In my changes, I have to change and verify all the includes/excludes
>> everywhere since I'm changing the semantics again. I would promise to keep
>> an extra eye on NativeThread.c.
>>> What is the actual problem with my fix? I could build without
>>> problems. And I still think EXCLUDE_FILE should just be just a file
>>> name. If you need to exclude a file by name without specifying a
>>> sub-directory you can build such a functionality into the new
>>> EXCLUDE_PATTERNS.
>> The problem is that we now have closed code where we already prepended a
>> slash as part of the change. If you change this behavior back, I will need
>> to quickly fix those closed places too. That is of course Oracle's problem
>> and not OpenJDK's.
>>
>> While I agree that a parameter named "EXCLUDE_FILES" should probably just
>> refer to exact filenames, I find the new semantics more useful, so the error
>> is rather in the parameter name. Also note that SetupJavaCompilation has
>> been working this way all along so this change was also intended to align
>> the semantics between the different macros.
>>> I really would appreciate a fast resolution of this problem because
>>> all our AIX builds are currently unusable.
>> I certainly understand that.
>>
>> /Erik
>>
>>> Thank you and best regards,
>>> Volker
>>>
>>>
>>> On Tue, Jan 5, 2016 at 9:53 AM, Erik Joelsson <erik.joelsson at oracle.com>
>>> wrote:
>>>>
>>>> On 2016-01-05 09:51, Erik Joelsson wrote:
>>>>> Hello Volker,
>>>>>
>>>>> I believe this change in behavior was intended at the time. In an
>>>>> internal
>>>>> situation where the same thing hit us, we changed the exclude pattern in
>>>>> question by adding a slash at the front: '/NativeThread.c'.
>>>>>
>>>> And the reason we needed this change was that we wanted to exclude files
>>>> by
>>>> name without having to specify sub directory. Typically in Hotspot, there
>>>> are a lot of sub directories with native source.
>>>>
>>>> /Erik
>>>>
>>>>> I should also inform you that I'm currently working on a major overhaul
>>>>> of
>>>>> all the INCLUDE/EXCLUDE file finding mechanisms in the build so that
>>>>> SetupNativeCompilation, SetupJavaCompilation, SetupCopyFiles etc all use
>>>>> the
>>>>> same semantics (and same implementation) for finding source files. I'm
>>>>> doing
>>>>> the work in the sandbox forest in a branch named
>>>>> erikj-makefilesets-branch
>>>>> if you are interested in checking it out. So far I've converted
>>>>> SetupCopyFiles and SetupTextFileProcessing. The common source file
>>>>> definitions can be found in the new FileSet.gmk:
>>>>>
>>>>>
>>>>>
>>>>> http://hg.openjdk.java.net/jdk9/sandbox/file/7ad550ee1d67/make/common/FileSet.gmk
>>>>>
>>>>> /Erik
>>>>>
>>>>> On 2016-01-04 20:26, Volker Simonis wrote:
>>>>>> Hi,
>>>>>>
>>>>>> could someone please review the following small fix:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8146425/
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8146425
>>>>>>
>>>>>> Change "8142907: Integration of minor fixes from the build-infra
>>>>>> project" has introduced a new parameter called EXCLUDE_PATTERN for
>>>>>> calls to SetupNativeCompilation.
>>>>>>
>>>>>> Unfortunately this change also altered the semantics of EXCLUDE_FILE
>>>>>> which is now interpreted as a pattern of the form "*EXCLUDE_FILE".
>>>>>> This is because of the following code:
>>>>>>
>>>>>>       ifneq ($$($1_ALL_EXCLUDE_FILES),)
>>>>>>         $1_EXCLUDE_FILES_PAT := $$($1_ALL_EXCLUDE_FILES) \
>>>>>>             $$(foreach i,$$($1_SRC),$$(addprefix
>>>>>> $$i/,$$($1_ALL_EXCLUDE_FILES)))
>>>>>>         $1_EXCLUDE_FILES_PAT := $$(addprefix %,$$($1_EXCLUDE_FILES_PAT))
>>>>>>         $1_SRCS := $$(filter-out $$($1_EXCLUDE_FILES_PAT),$$($1_SRCS))
>>>>>>
>>>>>> $1_EXCLUDE_FILES_PAT is initialized to contain all the simple file
>>>>>> names which are to be excluded plus all of these file names prefixed
>>>>>> with each src path. In the next step, all the entries in
>>>>>> $1_EXCLUDE_FILES_PAT are converted to patterns by prefixing them with
>>>>>> the wildcard character "%". Finally, the patterns are matched against
>>>>>> all the existing source files. This leads to the problem that every
>>>>>> file which was given as EXCLUDE_FILES will be converted into a
>>>>>> "%EXCLUDE_FILES" pattern thus effectively excluding not just files
>>>>>> with the name EXCLUDE_FILES but actually all files ending in
>>>>>> EXCLUDE_FILES.
>>>>>>
>>>>>> This hit us badly on AIX where we have the implementation file
>>>>>> AixNativeThread.c and the exclude NativeThread.c. After change 8142907
>>>>>> AixNativeThread.c was silently excluded from the compilation leading
>>>>>> to errors during runtime because the file contained some native
>>>>>> methods from sun.nio.ch which are not always used.
>>>>>>
>>>>>> Thank you and best regards,
>>>>>> Volker
>>>>>