Summary: | [Preferences] Adopt shared preferences configuration and script in WebKit | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | andersca, annulen, benjamin, cdumez, cmarcelo, darin, don.olmstead, ews-watchlist, gyuyoung.kim, ryuan.choi, sergio, stephan.szabo, thorton, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Sam Weinig
2020-09-28 19:11:20 PDT
Created attachment 409948 [details]
Patch
Created attachment 409954 [details]
Patch
Created attachment 409958 [details]
Patch
Created attachment 409992 [details]
Patch
Created attachment 409993 [details]
Patch
Any gtk or WPE folks that can help with the CMake part of this would be greatly appreciated. (In reply to Sam Weinig from comment #6) > Any gtk or WPE folks that can help with the CMake part of this would be > greatly appreciated. I think the problem is that the WTF yaml files are not getting copied to ${WTF_SCRIPTS_DIR}/Preferences/, but I don't know enough about CMake to know how to fix it. I tried adding a change to Source/WTF/wtf/CMakeLists.txt to copy the files, but I am guessing that is wrong. Created attachment 410035 [details]
Patch
Created attachment 410037 [details]
Patch
Created attachment 410041 [details]
Patch
Comment on attachment 410041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410041&action=review > Source/WTF/wtf/CMakeLists.txt:580 > +file(COPY Note that file(COPY) should be avoided if possible, because files are copied only when cmake is run and in incremental rebuilds they won't be updated until there are modifications in one of cmake files (In reply to Konstantin Tokarev from comment #11) > Comment on attachment 410041 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410041&action=review > > > Source/WTF/wtf/CMakeLists.txt:580 > > +file(COPY > > Note that file(COPY) should be avoided if possible, because files are copied > only when cmake is run and in incremental rebuilds they won't be updated > until there are modifications in one of cmake files Gotcha. What should be used instead in this case? Comment on attachment 410041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410041&action=review >>> Source/WTF/wtf/CMakeLists.txt:580 >>> +file(COPY >> >> Note that file(COPY) should be avoided if possible, because files are copied only when cmake is run and in incremental rebuilds they won't be updated until there are modifications in one of cmake files > > Gotcha. What should be used instead in this case? I saw you used WEBKIT_COPY_FILES in one of tries, it's much better because it creates makefile targets for copying files instead of copying them in cmake time But your cmake error was caused by cmake expecting it to be present before build, probably because of custom command referencing them as dependencies. I guess if target created by WEBKIT_COPY_FILES was a dependency of custom command instead of yaml files themselves, it would work properly. (In reply to Konstantin Tokarev from comment #13) > Comment on attachment 410041 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410041&action=review > > >>> Source/WTF/wtf/CMakeLists.txt:580 > >>> +file(COPY > >> > >> Note that file(COPY) should be avoided if possible, because files are copied only when cmake is run and in incremental rebuilds they won't be updated until there are modifications in one of cmake files > > > > Gotcha. What should be used instead in this case? > > I saw you used WEBKIT_COPY_FILES in one of tries, it's much better because > it creates makefile targets for copying files instead of copying them in > cmake time > > But your cmake error was caused by cmake expecting it to be present before > build, probably because of custom command referencing them as dependencies. > I guess if target created by WEBKIT_COPY_FILES was a dependency of custom > command instead of yaml files themselves, it would work properly. ok, so I go back to using this: set(WTF_SCRIPTS ../Scripts/GeneratePreferences.rb ../Scripts/generate-unified-source-bundles.rb ../Scripts/Preferences/WebPreferences.yaml ../Scripts/Preferences/WebPreferencesDebug.yaml ../Scripts/Preferences/WebPreferencesExperimental.yaml ../Scripts/Preferences/WebPreferencesInternal.yaml ) WEBKIT_COPY_FILES(WTF_CopyScripts DESTINATION ${WTF_SCRIPTS_DIR} FILES ${WTF_SCRIPTS} ) And then make the custom command in Source/WebKit/CMakeLists.txt that uses them depend on WTF_CopyScripts? Created attachment 410051 [details]
Patch
Created attachment 410053 [details]
Patch
(In reply to Sam Weinig from comment #16) > Created attachment 410053 [details] > Patch CMake Error in Source/WebKit/CMakeLists.txt: Cannot find source file: WTF_CopyScripts :( I think you should get rid of MAIN_DEPENDENCY, according to documentation it's expected to be file ("primary input source file to the command"), not a target I don't completely remember, but for the version using the file names in the dependency list you may need to add a set_property(SOURCE <names of the wtf yaml files> PROPERTY GENERATED 1) before that in that cmakelist file to make cmake realize they're generated since the custom command that generates them was not in the same or parent cmakelist. Comment on attachment 410051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410051&action=review > Source/WTF/wtf/CMakeLists.txt:584 > +WEBKIT_COPY_FILES(WTF_CopyScripts > + DESTINATION ${WTF_SCRIPTS_DIR} > + FILES ${WTF_SCRIPTS} > ) Let's change it to file(COPY ${WTF_SCRIPTS} DESTINATION ${WTF_SCRIPTS_DIR} ) Created attachment 410056 [details]
Patch
(In reply to Yusuke Suzuki from comment #20) > Comment on attachment 410051 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410051&action=review > > > Source/WTF/wtf/CMakeLists.txt:584 > > +WEBKIT_COPY_FILES(WTF_CopyScripts > > + DESTINATION ${WTF_SCRIPTS_DIR} > > + FILES ${WTF_SCRIPTS} > > ) > > Let's change it to > > file(COPY > ${WTF_SCRIPTS} > DESTINATION ${WTF_SCRIPTS_DIR} > ) Konstantin Tokarev argued against this above in https://bugs.webkit.org/show_bug.cgi?id=217075#c13 saying: "I saw you used WEBKIT_COPY_FILES in one of tries, it's much better because it creates makefile targets for copying files instead of copying them in cmake time" Created attachment 410057 [details]
Patch
Well, something is working! Thanks all. Created attachment 410059 [details]
Patch
Okiedoke. I think this is now ready for review. And it again uses file(COPY) with all consequences Did you get any errors with WEBKIT_COPY_FILES? I don't see any in EWS Yusuke got some errors locally when using it on a clean build due to the unified-source bundler needing one of the scripts. It seems possible we could WEBKIT_COPY_FILES for some of the files, but I don't know enough to understand if that would be worthwhile if have to use copy(FILE) for at least one. If this is about generate-unified-source-bundles.rb, I will readily believe it requires file(COPY) because unified-source bundler has to run at cmake time to generate new file lists for cmake (I don't exactly understand why that file can't be used in place though as ruby has -I, but that's a different matter) But I think there is no reason to copy all other files at cmake time. Also AFAIU preference files are to be changed much more often. Created attachment 410065 [details]
Patch
(In reply to Konstantin Tokarev from comment #30) > If this is about generate-unified-source-bundles.rb, I will readily believe > it requires file(COPY) because unified-source bundler has to run at cmake > time to generate new file lists for cmake (I don't exactly understand why > that file can't be used in place though as ruby has -I, but that's a > different matter) > > But I think there is no reason to copy all other files at cmake time. Also > AFAIU preference files are to be changed much more often. Ok, how does this latest one look to you? CMake parts lgtm Great, then now it's really ready for review! Committed r267775: <https://trac.webkit.org/changeset/267775> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410065 [details]. |