Closed Bug 1389257 Opened 2 years ago Closed 2 years ago

"ERROR: You must specify --with-pocket-api-keyfile" - use default Pocket API key file for dev builds

Categories

(Firefox for Android :: General, enhancement, P1)

ARM
Android
enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
1.27
Tracking Status
firefox57 --- fixed

People

(Reporter: liuche, Assigned: liuche)

References

Details

(Whiteboard: [MobileAS] 1.27)

Attachments

(1 file)

Dev builds don't reference the default pocket api key file, so builds fail.

Workaround right now is to add:

 ac_add_options --with-pocket-api-keyfile=$topsrcdir/mobile/android/base/pocket-api-sandbox.token
Assignee: nobody → liuche
Summary: Use default Pocket API key file for dev builds → "ERROR: You must specify --with-pocket-api-keyfile" - use default Pocket API key file for dev builds
This is fallout from bug 1386906, which works on the builders but not on dev builds.
Depends on: 1386906
Comment on attachment 8896069 [details]
Bug 1389257 - Set MOZ_ANDROID_POCKET only for nightly, beta, release builds.

https://reviewboard.mozilla.org/r/167350/#review172850

::: mobile/android/moz.configure:47
(Diff revision 2)
> +@depends(update_channel)
> +def pocket_stories_default(channel):
> +    return bool(channel == 'nightly' or channel == 'beta' or channel == 'release')
> +
>  option(env='MOZ_ANDROID_POCKET',
>         help='Enable Pocket Stories in Activity Stream.',
> -       default=True)
> +       default=pocket_stories_default)

We should be able to just set the default to `False` here. By setting `MOZ_ANDROID_POCKET` in the nightly mozconfig that should set it everywhere this condition would.
Attachment #8896069 - Flags: review?(cmanchester) → review+
This is just me being me, but wouldn't it be so much simpler and more flexible for developers to just change it such that specifying --with-pocket-api-keyfile results in enabling pocket and disabling it otherwise?
And before you asked where's the patch, there would have been this had this not currently been the most convoluted "logic" i have ever seen and I have not a clue how it is even expected to work.
(In reply to Bill Gianopoulos [:WG9s] from comment #5)
> This is just me being me, but wouldn't it be so much simpler and more
> flexible for developers to just change it such that specifying
> --with-pocket-api-keyfile results in enabling pocket and disabling it
> otherwise?

OK so this was a bit harsh, but the current enabling pocket is via an environment variable which only allows you to force it on if it otherwise would be off and no way to turn it off is really not  what is required.  Also having that via an environment variable but keyfile via mozconfig is also lame.  My suggestion would work without adding another variable, but the real solution would be to and a mozconfig option to enable/disable pocket and get rid of this ill functioning environment variable.
Sorry everyone for the breakage, obviously that wasn't the intended outcome :P I originally assumed that adding things to mozconfigs/common would add them in every build (including dev builds), rather than just the builders - what :WG9s is describing *was* the intent of the original patch, as patterned off some previous keyfile work. Basically, the overall goal is to get the builders building with the correct (secret) Pocket keys for each channel, where dev builds can optionally override the contents of the keyfile.

(In reply to Chris Manchester (:chmanchester) from comment #4)
> We should be able to just set the default to `False` here. By setting
> `MOZ_ANDROID_POCKET` in the nightly mozconfig that should set it everywhere
> this condition would.

Okay, that sounds good - I assume that means I should add this for all the mobile/android/config/mozconfigs/nightly* then. Are the nightly mozconfigs *also* are included in beta/release mozconfigs? I'll upload another patch, and make some sanity try pushes too.
(In reply to Chenxia Liu [:liuche] from comment #8)
> Sorry everyone for the breakage, obviously that wasn't the intended outcome
> :P I originally assumed that adding things to mozconfigs/common would add
> them in every build (including dev builds), rather than just the builders -
> what :WG9s is describing *was* the intent of the original patch, as
> patterned off some previous keyfile work. Basically, the overall goal is to
> get the builders building with the correct (secret) Pocket keys for each
> channel, where dev builds can optionally override the contents of the
> keyfile.
> 
> (In reply to Chris Manchester (:chmanchester) from comment #4)
> > We should be able to just set the default to `False` here. By setting
> > `MOZ_ANDROID_POCKET` in the nightly mozconfig that should set it everywhere
> > this condition would.
> 
> Okay, that sounds good - I assume that means I should add this for all the
> mobile/android/config/mozconfigs/nightly* then. Are the nightly mozconfigs
> *also* are included in beta/release mozconfigs? I'll upload another patch,
> and make some sanity try pushes too.

I figured that is what you were trying for it is just the original code is so strange.  you cannot define an environment variable MOZ_ANDROID_POCKET with a value of 0 to turn off pocket as an example of just how strange and intuitive the behavior is.
(In reply to Bill Gianopoulos [:WG9s] from comment #9)

> I figured that is what you were trying for it is just the original code is
> so strange.  you cannot define an environment variable MOZ_ANDROID_POCKET
> with a value of 0 to turn off pocket as an example of just how strange and
> intuitive the behavior is.

  non-intuitive
Just to make it clear we need 2 fixes here.  1 to make the builds work again and a second to fix this all to actually make some kind of logical sense.
Pushed by cliu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db3e18c3e3fa
Set MOZ_ANDROID_POCKET only for nightly, beta, release builds. r=chmanchester
I had to back this out for conflicting with my incoming merge from mozilla-central. Feel free to rebase/reland whenever.

https://hg.mozilla.org/integration/autoland/rev/b54b76d3dbd32e37d5642d42b18e55fc916ac948
Flags: needinfo?(liuche)
Pushed by cliu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eaf306c67af5
Set MOZ_ANDROID_POCKET only for nightly, beta, release builds. r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/eaf306c67af5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Flags: needinfo?(liuche)
You need to log in before you can comment on or make changes to this bug.