Bug 217075 – [Preferences] Adopt shared preferences configuration and script in WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[Preferences] Adopt shared preferences configuration and script in WebKit

[Sam Weinig]

Created attachment 409948 [details]
Patch

[Sam Weinig]

Created attachment 409954 [details]
Patch

[Sam Weinig]

Created attachment 409958 [details]
Patch

[Sam Weinig]

Created attachment 409992 [details]
Patch

[Sam Weinig]

Created attachment 409993 [details]
Patch

[Sam Weinig]

Any gtk or WPE folks that can help with the CMake part of this would be greatly appreciated.

[Sam Weinig]

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

[Sam Weinig]

Created attachment 410035 [details]
Patch

[Sam Weinig]

Created attachment 410037 [details]
Patch

[Sam Weinig]

Created attachment 410041 [details]
Patch

[Konstantin Tokarev]

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

[Sam Weinig]

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

[Konstantin Tokarev]

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.

[Sam Weinig]

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

[Sam Weinig]

Created attachment 410051 [details]
Patch

[Sam Weinig]

Created attachment 410053 [details]
Patch

[Sam Weinig]

(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

:(

[Konstantin Tokarev]

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

[Stephan Szabo]

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.

[Yusuke Suzuki]

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

[Sam Weinig]

Created attachment 410056 [details]
Patch

[Sam Weinig]

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

[Sam Weinig]

Created attachment 410057 [details]
Patch

[Sam Weinig]

Well, something is working! Thanks all.

[Sam Weinig]

Created attachment 410059 [details]
Patch

[Sam Weinig]

Okiedoke. I think this is now ready for review.

[Konstantin Tokarev]

And it again uses file(COPY) with all consequences

[Konstantin Tokarev]

Did you get any errors with WEBKIT_COPY_FILES? I don't see any in EWS

[Sam Weinig]

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.

[Konstantin Tokarev]

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.

[Sam Weinig]

Created attachment 410065 [details]
Patch

[Sam Weinig]

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

[Konstantin Tokarev]

CMake parts lgtm

[Sam Weinig]

Great, then now it's really ready for review!

[EWS]

Committed r267775: <https://trac.webkit.org/changeset/267775>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410065 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69772781>