Bug 211781 – [Flatpak SDK] Craft a custom sccache config file from SDK toolchains

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Philippe Normand]

`webkit-flatpak -r -t <token>` would generate a config file in WebKitBuild/UserFlatpak/. This would be optional though. A global user config file (from ~/.config/sccache/) can still be used if the generated config doesn't exist.

[Philippe Normand]

Created attachment 399128 [details]
Patch

[Pablo Saavedra]

Comment on attachment 399128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399128&action=review

> Tools/flatpak/flatpakutils.py:873
> +            sccache_config = {'dist': {'scheduler_url': 'https://sccache.igalia.com',

What about?:

  `sccache_config = {'dist': {'scheduler_url': sccache_scheduler,`

with:

  `general.add_argument("-s", "--sccache-scheduler", dest="sccache_scheduler", default='https://sccache.igalia.com')`

[Carlos Alberto Lopez Perez]

Comment on attachment 399128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399128&action=review

> Tools/flatpak/flatpakutils.py:878
> +        with open(self.sccache_config_file, 'w') as config:
> +            sccache_config = {'dist': {'scheduler_url': 'https://sccache.igalia.com',
> +                                       'toolchain_cache_size': 5368709120,
> +                                       'auth': {'type': 'token',
> +                                                'token': self.sccache_token},
> +                                       'toolchains': toolchains}}
> +            toml.dump(sccache_config, config)

To me it feels overkill to add a new dependency (toml) to just be able to write the sccache config in json/dict format instead of writing it directly in its own native toml format.
Its there any reason to not write the config file directly in its own native format (avoiding this dependency)?
You can use a templated variable to make that more pretty. Example:

sccache_config_template = """\
[dist]
scheduler_url = "%(scheduler_url)s"
toolchain_cache_size = 5368709120
toolchains = "%(toolchains)s"

[dist.auth]
token = "%(sccache_token)s"
type = "token"
"""


print (sccache_config_template % {
           "scheduler_url" : "https://sccache.igalia.com",
           "toolchains" : toolchains,
           "sccache_token" : self.sccache_token} )

[Philippe Normand]

Comment on attachment 399128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399128&action=review

>> Tools/flatpak/flatpakutils.py:878
>> +            toml.dump(sccache_config, config)
> 
> To me it feels overkill to add a new dependency (toml) to just be able to write the sccache config in json/dict format instead of writing it directly in its own native toml format.
> Its there any reason to not write the config file directly in its own native format (avoiding this dependency)?
> You can use a templated variable to make that more pretty. Example:
> 
> sccache_config_template = """\
> [dist]
> scheduler_url = "%(scheduler_url)s"
> toolchain_cache_size = 5368709120
> toolchains = "%(toolchains)s"
> 
> [dist.auth]
> token = "%(sccache_token)s"
> type = "token"
> """
> 
> 
> print (sccache_config_template % {
>            "scheduler_url" : "https://sccache.igalia.com",
>            "toolchains" : toolchains,
>            "sccache_token" : self.sccache_token} )

You'd need a template for the toolchains too. My solution looks simpler imho :) And toml is a small python package anyway, automatically installed. I don't buy the overkill argument :)

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #4)
> Comment on attachment 399128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399128&action=review
> 
> >> Tools/flatpak/flatpakutils.py:878
> >> +            toml.dump(sccache_config, config)
> > 
> > To me it feels overkill to add a new dependency (toml) to just be able to write the sccache config in json/dict format instead of writing it directly in its own native toml format.
> > Its there any reason to not write the config file directly in its own native 
> 
> You'd need a template for the toolchains too. 

Right.. i missed that

> My solution looks simpler imho
> :) And toml is a small python package anyway, automatically installed. I
> don't buy the overkill argument :)

ok, fair enough

[Carlos Alberto Lopez Perez]

Comment on attachment 399128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399128&action=review

>> Tools/flatpak/flatpakutils.py:873
>> +            sccache_config = {'dist': {'scheduler_url': 'https://sccache.igalia.com',
> 
> What about?:
> 
>   `sccache_config = {'dist': {'scheduler_url': sccache_scheduler,`
> 
> with:
> 
>   `general.add_argument("-s", "--sccache-scheduler", dest="sccache_scheduler", default='https://sccache.igalia.com')`

I like this suggestion of making the url configurable

> Tools/flatpak/flatpakutils.py:874
> +                                       'toolchain_cache_size': 5368709120,

The default seems to be 10GB. Why lowering it to 5GB?

[Pablo Saavedra]

(In reply to Philippe Normand from comment #4)
> Comment on attachment 399128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399128&action=review
> 
> >> Tools/flatpak/flatpakutils.py:878
> >> +            toml.dump(sccache_config, config)
> > 
> > To me it feels overkill to add a new dependency (toml) to just be able to write the sccache config in json/dict format instead of writing it directly in its own native toml format.
> > Its there any reason to not write the config file directly in its own native format (avoiding this dependency)?
> > You can use a templated variable to make that more pretty. Example:
> > 
> > sccache_config_template = """\
> > [dist]
> > scheduler_url = "%(scheduler_url)s"
> > toolchain_cache_size = 5368709120
> > toolchains = "%(toolchains)s"
> > 
> > [dist.auth]
> > token = "%(sccache_token)s"
> > type = "token"
> > """
> > 
> > 
> > print (sccache_config_template % {
> >            "scheduler_url" : "https://sccache.igalia.com",
> >            "toolchains" : toolchains,
> >            "sccache_token" : self.sccache_token} )
> 
> You'd need a template for the toolchains too. My solution looks simpler imho
> :) And toml is a small python package anyway, automatically installed. I
> don't buy the overkill argument :)

Just a simple comment is not configparser enough to built this particular file template?

[Pablo Saavedra]

*build

[Pablo Saavedra]

(In reply to Pablo Saavedra from comment #7)
> (In reply to Philippe Normand from comment #4)
> > Comment on attachment 399128 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=399128&action=review
> > 
> > >> Tools/flatpak/flatpakutils.py:878
> > >> +            toml.dump(sccache_config, config)
> > > 
> > > To me it feels overkill to add a new dependency (toml) to just be able to write the sccache config in json/dict format instead of writing it directly in its own native toml format.
> > > Its there any reason to not write the config file directly in its own native format (avoiding this dependency)?
> > > You can use a templated variable to make that more pretty. Example:
> > > 
> > > sccache_config_template = """\
> > > [dist]
> > > scheduler_url = "%(scheduler_url)s"
> > > toolchain_cache_size = 5368709120
> > > toolchains = "%(toolchains)s"
> > > 
> > > [dist.auth]
> > > token = "%(sccache_token)s"
> > > type = "token"
> > > """
> > > 
> > > 
> > > print (sccache_config_template % {
> > >            "scheduler_url" : "https://sccache.igalia.com",
> > >            "toolchains" : toolchains,
> > >            "sccache_token" : self.sccache_token} )
> > 
> > You'd need a template for the toolchains too. My solution looks simpler imho
> > :) And toml is a small python package anyway, automatically installed. I
> > don't buy the overkill argument :)
> 
> Just a simple comment is not configparser enough to built this particular
> file template?


Nevermind. I see the resulting file differs too much than a regular INI file:

[dist]
scheduler_url = "https://sccache.igalia.com"
toolchain_cache_size = 5368709120
[[dist.toolchains]]
compiler_executable = "/usr/bin/c++"
archive_compiler_executable = "/usr/bin/g++"
type = "path_override"
archive = "/home/psaavedra/local/git/webkit-gtk/WebKit/WebKitBuild/Toolchains/webkit-sdk-gcc-c636916ef47c8b977cbfab46ae5f3289.tar.gz"

[[dist.toolchains]]
compiler_executable = "/usr/bin/clang++"
archive_compiler_executable = "/usr/bin/clang++"
type = "path_override"
archive = "/home/psaavedra/local/git/webkit-gtk/WebKit/WebKitBuild/Toolchains/webkit-sdk-clang-a0c3c4672a324be2250371fbe01c10fb.tar.gz"

[dist.auth]
token = "XXXXXXXXXXXXXXXXXXX"
type = "token"

[Philippe Normand]

(In reply to Carlos Alberto Lopez Perez from comment #6)

> 
> I like this suggestion of making the url configurable
> 

Ok, let's do it then.

> > Tools/flatpak/flatpakutils.py:874
> > +                                       'toolchain_cache_size': 5368709120,
> 
> The default seems to be 10GB. Why lowering it to 5GB?

No reason, I'll remove this :)

[Philippe Normand]

Committed r261607: <https://trac.webkit.org/changeset/261607>

[Radar WebKit Bug Importer]

<rdar://problem/63175727>