When a change contains a WebCore change, one or more added layout tests, and a PAL change then prepare-ChangeLog will list the added layout tests in both the WebCore and PAL ChangeLog files. Although platform functionality in PAL may be directly exercised in a layout test I do not get the impression that a layout test can always be used to exercise PAL functionality or at least exercise all edge cases easily. Unit tests seem more appropriate for abstractions in PAL. Therefore, prepare-ChangeLog should not list added layout tests in the PAL ChangeLog.
Created attachment 316488 [details] Patch
Comment on attachment 316488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316488&action=review > Tools/Scripts/prepare-ChangeLog:691 > + if (unixPath($prefix) =~ m|WebCore(?!/PAL)| || unixPath(`pwd`) =~ m|WebCore(?!/PAL)| || @$requiresTests) { I don’t think this fix need to be so specific to PAL.
(In reply to Darin Adler from comment #2) > Comment on attachment 316488 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=316488&action=review > > > Tools/Scripts/prepare-ChangeLog:691 > > + if (unixPath($prefix) =~ m|WebCore(?!/PAL)| || unixPath(`pwd`) =~ m|WebCore(?!/PAL)| || @$requiresTests) { > > I don’t think this fix need to be so specific to PAL. I am unclear how to avoid making this fix specific to PAL for the second disjunct without breaking the prepare-ChangeLog feature implemented by the second disjunct when the current working directory is some non-PAL subdirectory of Source/WebCore. If we do not need to support this feature then I can remove the second disjunct and write the first disjunct as "unixPath($prefix) =~ m|WebCore/$|" (without double quotes). The purpose of the second disjunct is to always (*) emit "No new tests (OOPS!)." in the WebCore ChangeLog entry whenever a person runs prepare-ChangeLog inside Source/WebCore or a subdirectory of it. I suspect the rationality (**) behind this behavior is to support the workflow of some people that want to generate a ChangeLog and/or patch from a subdirectory of their checkout either because they have unrelated changes in another part of their checkout or as a performance optimization to avoid stat'ing their entire checkout. For people with this workflow prepare-ChangeLog wants to hint to them that they either must by hand remove the "No new tests (OOPS!)." line and list the added tests or provide a reason for the omission of tests OR run prepare-ChangeLog from the top-level WebKit checkout directory so that prepare-ChangeLog can find any added tests. Do we need to support this feature? If so, do we consider the feature meaningful if its limited to when the current working directory is Source/WebCore and not a subdirectory of it? Do you have any insight/suggestions on how to fix up the second disjunct without being specific to PAL or enumerating all other (or most) subdirectories of Source/WebCore? (*) We will always emit this line because @addedRegressionTests will always be empty since we only add an item to this list if prepare-ChangeLog saw an added file to LayoutTests. And prepare-ChangeLog only sees files that are in the current working directory or subdirectories of it. (**) This code has existed since prepare-ChangeLog was added by Maciej Stachowiak in <https://trac.webkit.org/changeset/9406>. Maybe he still recalls the reason he added this code. CC'ed Maciej.
(In reply to Daniel Bates from comment #3) > For people with this workflow prepare-ChangeLog wants to hint to them ... This should read: For people with this workflow and run prepare-ChangeLog from Source/WebCore or a subdirectory of it prepare-ChangeLog wants to hint to them ...
Comment on attachment 316488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316488&action=review >>> Tools/Scripts/prepare-ChangeLog:691 >>> + if (unixPath($prefix) =~ m|WebCore(?!/PAL)| || unixPath(`pwd`) =~ m|WebCore(?!/PAL)| || @$requiresTests) { >> >> I don’t think this fix need to be so specific to PAL. > > I am unclear how to avoid making this fix specific to PAL for the second disjunct without breaking the prepare-ChangeLog feature implemented by the second disjunct when the current working directory is some non-PAL subdirectory of Source/WebCore. If we do not need to support this feature then I can remove the second disjunct and write the first disjunct as "unixPath($prefix) =~ m|WebCore/$|" (without double quotes). The purpose of the second disjunct is to always (*) emit "No new tests (OOPS!)." in the WebCore ChangeLog entry whenever a person runs prepare-ChangeLog inside Source/WebCore or a subdirectory of it. I suspect the rationality (**) behind this behavior is to support the workflow of some people that want to generate a ChangeLog and/or patch from a subdirectory of their checkout either because they have unrelated changes in another part of their checkout or as a performance optimization to avoid stat'ing their entire checkout. For people with this workflow prepare-ChangeLog wants to hint to them that they either must by hand remove the "No new tests (OOPS!)." line and list the added tests or provide a reason for the omission of tests OR run prepare-ChangeLog from the top-level WebKit checkout directory so that prepare-ChangeLog can find any added tests. Do we need to support this feature? If so, do we consider the feature meaningful if its limited to when the current working directory is Source/WebCore and not a subdirectory of it? Do you have any insight/suggestions on how to fix up the second disjunct without being specific to PAL or enumerating all other (or most) subdirectories of Source/WebCore? > > (*) We will always emit this line because @addedRegressionTests will always be empty since we only add an item to this list if prepare-ChangeLog saw an added file to LayoutTests. And prepare-ChangeLog only sees files that are in the current working directory or subdirectories of it. > > (**) This code has existed since prepare-ChangeLog was added by Maciej Stachowiak in <https://trac.webkit.org/changeset/9406>. Maybe he still recalls the reason he added this code. CC'ed Maciej. Yes, I want you to do what you mentioned in the first sentence. if (unixPath($prefix) =~ m|/WebCore/$| || @$requiresTests) { The same bug affects WebCore/platform/gtk/po/ChangeLog and your fix would not help in that case, but mine would. Can you give an example of what you think this breaks about subdirectories? Just give me a specific example. No need for rationale or philosophy. In what case is the behavior different? Steps to reproduce. I tried and could not reproduce any difference in behavior.
(In reply to Darin Adler from comment #5) > Can you give an example of what you think this breaks about subdirectories? > Just give me a specific example. No need for rationale or philosophy. In > what case is the behavior different? Steps to reproduce. I tried and could > not reproduce any difference in behavior. Never mind. I misread the code in findChangeLogs() and for some reason I thought $prefix was the full path to a modified file.
Created attachment 316520 [details] Patch
Comment on attachment 316520 [details] Patch Attachment 316520 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4194715 New failing tests: fast/selectors/nth-last-child-of-selector-list-ending-with-never-matching-selectors.html
Created attachment 316527 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
(In reply to Build Bot from comment #8) > Comment on attachment 316520 [details] > Patch > > Attachment 316520 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: http://webkit-queues.webkit.org/results/4194715 > > New failing tests: > fast/selectors/nth-last-child-of-selector-list-ending-with-never-matching- > selectors.html Clearly this failure is unrelated to this patch as this patch does not touch the engine.
Comment on attachment 316520 [details] Patch Clearing flags on attachment: 316520 Committed r220032: <http://trac.webkit.org/changeset/220032>
All reviewed patches have been landed. Closing bug.
<rdar://problem/33601796>