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.joels...@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.joels...@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.joels...@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 >>>>>> >>>>>> >
