Bug 217075 - [Preferences] Adopt shared preferences configuration and script in WebKit
Summary: [Preferences] Adopt shared preferences configuration and script in WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-28 19:11 PDT by Sam Weinig
Modified: 2020-09-29 18:30 PDT (History)
15 users (show)

See Also:


Attachments
Patch (118.22 KB, patch)
2020-09-28 19:14 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (118.50 KB, patch)
2020-09-28 19:46 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (118.69 KB, patch)
2020-09-28 20:03 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (117.75 KB, patch)
2020-09-29 06:10 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (118.54 KB, patch)
2020-09-29 06:32 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (117.77 KB, patch)
2020-09-29 13:23 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (117.88 KB, patch)
2020-09-29 13:34 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (117.74 KB, patch)
2020-09-29 13:45 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (117.87 KB, patch)
2020-09-29 14:25 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (117.78 KB, patch)
2020-09-29 14:34 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (117.76 KB, patch)
2020-09-29 14:50 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (117.94 KB, patch)
2020-09-29 14:57 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (120.58 KB, patch)
2020-09-29 15:34 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (120.63 KB, patch)
2020-09-29 16:23 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-09-28 19:11:20 PDT
[Preferences] Adopt shared preferences configuration and script in WebKit
Comment 1 Sam Weinig 2020-09-28 19:14:30 PDT Comment hidden (obsolete)
Comment 2 Sam Weinig 2020-09-28 19:46:09 PDT Comment hidden (obsolete)
Comment 3 Sam Weinig 2020-09-28 20:03:11 PDT Comment hidden (obsolete)
Comment 4 Sam Weinig 2020-09-29 06:10:42 PDT Comment hidden (obsolete)
Comment 5 Sam Weinig 2020-09-29 06:32:43 PDT Comment hidden (obsolete)
Comment 6 Sam Weinig 2020-09-29 08:46:52 PDT
Any gtk or WPE folks that can help with the CMake part of this would be greatly appreciated.
Comment 7 Sam Weinig 2020-09-29 11:59:49 PDT
(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.
Comment 8 Sam Weinig 2020-09-29 13:23:30 PDT Comment hidden (obsolete)
Comment 9 Sam Weinig 2020-09-29 13:34:03 PDT Comment hidden (obsolete)
Comment 10 Sam Weinig 2020-09-29 13:45:30 PDT Comment hidden (obsolete)
Comment 11 Konstantin Tokarev 2020-09-29 13:48:34 PDT
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
Comment 12 Sam Weinig 2020-09-29 13:59:02 PDT
(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 13 Konstantin Tokarev 2020-09-29 14:11:38 PDT
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.
Comment 14 Sam Weinig 2020-09-29 14:21:06 PDT
(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?
Comment 15 Sam Weinig 2020-09-29 14:25:09 PDT Comment hidden (obsolete)
Comment 16 Sam Weinig 2020-09-29 14:34:19 PDT
Created attachment 410053 [details]
Patch
Comment 17 Sam Weinig 2020-09-29 14:35:55 PDT
(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

:(
Comment 18 Konstantin Tokarev 2020-09-29 14:38:35 PDT
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
Comment 19 Stephan Szabo 2020-09-29 14:40:39 PDT
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 20 Yusuke Suzuki 2020-09-29 14:45:47 PDT
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}
)
Comment 21 Sam Weinig 2020-09-29 14:50:40 PDT Comment hidden (obsolete)
Comment 22 Sam Weinig 2020-09-29 14:55:05 PDT
(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"
Comment 23 Sam Weinig 2020-09-29 14:57:49 PDT Comment hidden (obsolete)
Comment 24 Sam Weinig 2020-09-29 15:14:31 PDT
Well, something is working! Thanks all.
Comment 25 Sam Weinig 2020-09-29 15:34:05 PDT
Created attachment 410059 [details]
Patch
Comment 26 Sam Weinig 2020-09-29 15:40:50 PDT
Okiedoke. I think this is now ready for review.
Comment 27 Konstantin Tokarev 2020-09-29 15:53:47 PDT
And it again uses file(COPY) with all consequences
Comment 28 Konstantin Tokarev 2020-09-29 16:01:07 PDT
Did you get any errors with WEBKIT_COPY_FILES? I don't see any in EWS
Comment 29 Sam Weinig 2020-09-29 16:04:19 PDT
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.
Comment 30 Konstantin Tokarev 2020-09-29 16:11:45 PDT
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.
Comment 31 Sam Weinig 2020-09-29 16:23:51 PDT
Created attachment 410065 [details]
Patch
Comment 32 Sam Weinig 2020-09-29 16:24:19 PDT
(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?
Comment 33 Konstantin Tokarev 2020-09-29 16:48:41 PDT
CMake parts lgtm
Comment 34 Sam Weinig 2020-09-29 17:09:17 PDT
Great, then now it's really ready for review!
Comment 35 EWS 2020-09-29 18:29:47 PDT
Committed r267775: <https://trac.webkit.org/changeset/267775>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410065 [details].
Comment 36 Radar WebKit Bug Importer 2020-09-29 18:30:36 PDT
<rdar://problem/69772781>