1294585 - API key files configure move/cleanup

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

+++ This bug was initially created as a clone of Bug #1256730 +++

Bug 1256730 is blocked on bug 1293643, but only a small patch out of it is. So let's land all the rest in this clone.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1294585 - Remove --with-google-oauth-api-keyfile from mozconfigs.

The flag has not had an effect since bug 1213984, and the file given as
argument is not even on the build slaves anymore.

Review commit: https://reviewboard.mozilla.org/r/71120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71120/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1294585 - Remove the --with-google-oauth-api-keyfile configure flag.

Review commit: https://reviewboard.mozilla.org/r/71122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71122/

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1294585 - Move --with-*-keyfile options to python configure.

Review commit: https://reviewboard.mozilla.org/r/71124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71124/

Comment 4

[Mike Hommey [:glandium]]

Sorry Mike, those are the same patches as you reviewed in bug 1256730, but mozreview doesn't know about that.

Comment 5

[Mike Hommey [:glandium]]

To Chris: writing a test case made me find a couple problems that made me change the check substantially. Yay for testable configure.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8780391 [details]
Bug 1294585 - Remove --with-google-oauth-api-keyfile from mozconfigs.

https://reviewboard.mozilla.org/r/71120/#review68644

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8780392 [details]
Bug 1294585 - Remove the --with-google-oauth-api-keyfile configure flag.

https://reviewboard.mozilla.org/r/71122/#review68646

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8780392 [details]
Bug 1294585 - Remove the --with-google-oauth-api-keyfile configure flag.

https://reviewboard.mozilla.org/r/71122/#review68648

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8780393 [details]
Bug 1294585 - Move --with-*-keyfile options to python configure.

https://reviewboard.mozilla.org/r/71124/#review68678

::: build/moz.configure/keyfiles.configure:7
(Diff revision 1)
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +

I think you can drop one of those empty lines

::: build/moz.configure/keyfiles.configure:29
(Diff revision 1)
> +                    result = fh.read().strip()
> +                    if result:
> +                        return callback(result)
> +            except FatalCheckError:
> +                raise
> +            except:

Squashing any exception at all seems kinda fishy.

::: mobile/android/base/moz.build:968
(Diff revision 1)
>          # it in the main APK as well.
>          ANDROID_ASSETS_DIRS += [
>              '%' + CONFIG['MOZ_ANDROID_DISTRIBUTION_DIRECTORY'] + '/assets',
>          ]
>  
> -# We do not expose MOZ_INSTALL_TRACKING_ADJUST_SDK_APP_TOKEN here because that
> +# We do not expose MOZ_ADJUST_SDK_KEY here because that # would leak the value

Extraneous `#`

Comment 10

[Mike Hommey [:glandium]]

(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #9)
> ::: build/moz.configure/keyfiles.configure:29
> (Diff revision 1)
> > +                    result = fh.read().strip()
> > +                    if result:
> > +                        return callback(result)
> > +            except FatalCheckError:
> > +                raise
> > +            except:
> 
> Squashing any exception at all seems kinda fishy.

No, it's on purpose, because the file might not exist. Fixing that is bug 1256730

Comment 11

[Justin Wood (:Callek)]

Comment on attachment 8780393 [details]
Bug 1294585 - Move --with-*-keyfile options to python configure.

https://reviewboard.mozilla.org/r/71124/#review68678

> Squashing any exception at all seems kinda fishy.

I agree here though... For example:

```
>>> while 1:
...   try:
...      time.sleep(3)
...   except:
...      print "Haha, no ^C allowed"
...   "still going..."
... 
'still going...'
^CHaha, no ^C allowed
'still going...'
^CHaha, no ^C allowed
'still going...'
^CHaha, no ^C allowed
'still going...'
^CHaha, no ^C allowed
'still going...'
^CHaha, no ^C allowed
'still going...'
```

however using ```except Exception:``` protects against file not found and still allows ^C through, because `KeyboardError` is not subclassed from Exception (for exactly this reason)

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8780393 [details]
Bug 1294585 - Move --with-*-keyfile options to python configure.

https://reviewboard.mozilla.org/r/71124/#review68878

::: mobile/android/base/adjust_sdk_app_token.in:2
(Diff revision 1)
>  //#ifdef MOZ_INSTALL_TRACKING
> -//#define MOZ_INSTALL_TRACKING_ADJUST_SDK_APP_TOKEN @MOZ_INSTALL_TRACKING_ADJUST_SDK_APP_TOKEN@
> +//#define MOZ_INSTALL_TRACKING_ADJUST_SDK_APP_TOKEN @MOZ_SDK_APP_KEY@

s/MOZ_SDK_APP_KEY/MOZ_ADJUST_SDK_KEY/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a49d0290fab
Remove --with-google-oauth-api-keyfile from mozconfigs. r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa14d58c7141
Remove the --with-google-oauth-api-keyfile configure flag. r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/a88047e08c2f
Move --with-*-keyfile options to python configure. r=chmanchester

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6a49d0290fab
https://hg.mozilla.org/mozilla-central/rev/aa14d58c7141
https://hg.mozilla.org/mozilla-central/rev/a88047e08c2f