Bug 211781 - [Flatpak SDK] Craft a custom sccache config file from SDK toolchains
Summary: [Flatpak SDK] Craft a custom sccache config file from SDK toolchains
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-12 08:46 PDT by Philippe Normand
Modified: 2020-05-13 01:59 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.72 KB, patch)
2020-05-12 08:50 PDT, Philippe Normand
clopez: review+
clopez: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2020-05-12 08:46:54 PDT
`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.
Comment 1 Philippe Normand 2020-05-12 08:50:49 PDT
Created attachment 399128 [details]
Patch
Comment 2 Pablo Saavedra 2020-05-12 09:13:54 PDT
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')`
Comment 3 Carlos Alberto Lopez Perez 2020-05-12 09:48:08 PDT
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} )
Comment 4 Philippe Normand 2020-05-12 09:58:13 PDT
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 :)
Comment 5 Carlos Alberto Lopez Perez 2020-05-12 10:13:40 PDT
(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
Comment 6 Carlos Alberto Lopez Perez 2020-05-12 10:15:20 PDT
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?
Comment 7 Pablo Saavedra 2020-05-12 10:18:16 PDT
(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?
Comment 8 Pablo Saavedra 2020-05-12 10:19:17 PDT
*build
Comment 9 Pablo Saavedra 2020-05-12 10:26:15 PDT
(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"
Comment 10 Philippe Normand 2020-05-13 01:43:37 PDT
(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 :)
Comment 11 Philippe Normand 2020-05-13 01:58:15 PDT
Committed r261607: <https://trac.webkit.org/changeset/261607>
Comment 12 Radar WebKit Bug Importer 2020-05-13 01:59:16 PDT
<rdar://problem/63175727>