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

Tue Jan 5 17:50:51 UTC 2016

Thanks for the fast review Erik.
I'll push it now and if I'm lucky enough I'll already fix our nightly
builds today :)

Regards,
Volker

On Tue, Jan 5, 2016 at 6:49 PM, Erik Joelsson <erik.joelsson at oracle.com> wrote:
> 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
>>>>>>
>>>>>>
>