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

Tue Jan 5 17:42:06 UTC 2016

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
>>>>
>>>>
>