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

[Erik Joelsson]

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