Bug 211781

Created attachment 399128 [details]
Patch

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

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

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

*build

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

(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 :)

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

<rdar://problem/63175727>