Closed Bug 1294585 Opened 3 years ago Closed 3 years ago

API key files configure move/cleanup

Categories

(Firefox Build System :: General, defect)

43 Branch
defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

+++ 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.
Sorry Mike, those are the same patches as you reviewed in bug 1256730, but mozreview doesn't know about that.
To Chris: writing a test case made me find a couple problems that made me change the check substantially. Yay for testable configure.
Comment on attachment 8780391 [details]
Bug 1294585 - Remove --with-google-oauth-api-keyfile from mozconfigs.

https://reviewboard.mozilla.org/r/71120/#review68644
Attachment #8780391 - Flags: review?(mdeboer) → review+
Comment on attachment 8780392 [details]
Bug 1294585 - Remove the --with-google-oauth-api-keyfile configure flag.

https://reviewboard.mozilla.org/r/71122/#review68646
Comment on attachment 8780392 [details]
Bug 1294585 - Remove the --with-google-oauth-api-keyfile configure flag.

https://reviewboard.mozilla.org/r/71122/#review68648
Attachment #8780392 - Flags: review?(mdeboer) → review+
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
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 `#`
(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 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 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/
Attachment #8780393 - Flags: review?(cmanchester) → review+
Pushed by mh@glandium.org:
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
https://hg.mozilla.org/mozilla-central/rev/6a49d0290fab
https://hg.mozilla.org/mozilla-central/rev/aa14d58c7141
https://hg.mozilla.org/mozilla-central/rev/a88047e08c2f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Blocks: 1317359
Depends on: 1333516
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 51 → mozilla51
You need to log in before you can comment on or make changes to this bug.