Closed Bug 1256730 Opened 8 years ago Closed 7 years ago

Fail if api key files don't exist

Categories

(Firefox Build System :: General, defect)

43 Branch
defect
Not set
normal

Tracking

(firefox52 fixed, firefox53 fixed, firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: dustin, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

In bug 1231320, I arranged to provide various API keyfiles in /builds to all build tasks, including try (level-1) builds.  The latter have "fake" secrets, though.

The bug came to us a bit late because the build process did not complain that the secrets files named in the mozconfigs didn't exist, and went on blithely building without secrets.  I'd rather that not happen again.  The obvious fix is to fail configure in that condition.

However, try builds under Buildbot do, in fact, lack these files, so the obvious fix makes such builds fail.  Mozconfigs have no way to know the scm level they're building, and it feels wrong to inject that information.

So, let's wait until we're not building in Buildbot anymore, then land the change.
Attached patch bug1256730.patch (obsolete) — Splinter Review
r+ from glandium in bug 1231320
Attachment #8730818 - Flags: review+
(re-requesting review since this has been 5mo since r+)
Comment on attachment 8779006 [details]
Bug 1256730: fail configure when api keyfiles do not exist;

https://reviewboard.mozilla.org/r/70070/#review67560
Attachment #8779006 - Flags: review?(mh+mozilla) → review-
Assignee: dustin → mh+mozilla
Blocks: pyconfigure
(In reply to Dustin J. Mitchell [:dustin] from comment #3)
> (re-requesting review since this has been 5mo since r+)

Is something supposed to have changed wrt the key files? Because try tells me the second patch breaks the build on TC because /builds/google-oauth-api.key is not there.
It's actually funnier than that, it's missing on buildbot builds too.
And it's not even a try vs non-try thing... inbound builds have this in nsURLFormatter.js:
GOOGLE_OAUTH_API_CLIENTID:() => "no-google-oauth-api-clientid",
GOOGLE_OAUTH_API_KEY:     () => "no-google-oauth-api-key",
BING_API_CLIENTID:() => "no-bing-api-clientid",
BING_API_KEY:     () => "no-bing-api-key",
The google oauth api key used to be there and disappeared in Firefox 46. The only code that used it was actually removed in Firefox 45 (bug 1213984)... I guess someone thought about removing the files on build slaves, but not the build system stuff...

The bing API key was added in bug 1014367 and has never shipped a value. At least, mozconfigs are not trying to pass the option with a non-existing file...
The Google API key was used by the Hello import contacts feature, which is now obsolete.
The Bing API key was used for the in-page content translation feature, which we haven't shipped (yet).
Comment on attachment 8779261 [details]
Bug 1256730 - Fail configure when API key files do not exist or are empty.

https://reviewboard.mozilla.org/r/70288/#review67582

Ship it!
Attachment #8779261 - Flags: review?(mdeboer) → review+
Comment on attachment 8779262 [details]
Bug 1256730 - Add Mozilla and Google API files to artifact and cross-osx builds.

https://reviewboard.mozilla.org/r/70290/#review67584
Attachment #8779262 - Flags: review?(mdeboer) → review+
Looks like this is all not enough. New problem: Windows try buildbot slaves don't have the mozilla api key file (but it's there on inbound)
Flags: needinfo?(dustin)
Well, I'm glad I re-requested review!

If it's missing on Windows try slaves, then we should get another bug on file to fix that (bug 1293643).  Hopefully that's just try slaves, though, and we haven't adversely impacted any shipped releases with that error!
Depends on: 1293643
Flags: needinfo?(dustin)
Attachment #8730818 - Attachment is obsolete: true
Comment on attachment 8779298 [details]
Bug 1256730 - Add Mozilla and Google API files to artifact builds.

https://reviewboard.mozilla.org/r/70310/#review67598
Attachment #8779298 - Flags: review?(dustin) → review+
Comment on attachment 8779292 [details]
Bug 1256730 - Fail configure when API keyfiles do not exist or are empty.

https://reviewboard.mozilla.org/r/70308/#review68084
Attachment #8779292 - Flags: review?(cmanchester) → review+
Comment on attachment 8779291 [details]
Bug 1256730 - Move --with-*-keyfile options to python configure.

https://reviewboard.mozilla.org/r/70306/#review68082

Looks great. Clearing review because however we handle the adjust sdk variable name might be enough to warrant re-review.

::: build/moz.configure/keyfiles.configure:49
(Diff revision 1)
> +    def id_and_secret(value):
> +        if value.startswith('no-') and value.endswith('-key'):
> +            id = value[:-3] + 'clientid'
> +            secret = value
> +        else:
> +            id, secret = value[0].split(' ', 1)

Shouldn't this just be `value`?

::: build/moz.configure/keyfiles.configure:66
(Diff revision 1)
> +
> +simple_keyfile('Google API')
> +
> +id_and_secret_keyfile('Bing API')
> +
> +simple_keyfile('Adjust SDK')

The old variable for this is `MOZ_INSTALL_TRACKING_ADJUST_SDK_APP_TOKEN`, but the trasnposition in `simple_keyfile` will give us `MOZ_ADJUST_SDK_TOKEN`. Should we just change the consumers to match?
Attachment #8779291 - Flags: review?(cmanchester)
Depends on: 1294585
Attachment #8779291 - Attachment is obsolete: true
Attachment #8779292 - Attachment is obsolete: true
Attachment #8779298 - Attachment is obsolete: true
Sorry Dustin, this is the same patch as previously, but mozreview doesn't know.
Chris, this is rebased on top of bug 1294585, with a testcase, so the re-review is wanted.
Attachment #8779262 - Flags: review?(dustin) → review+
Comment on attachment 8779261 [details]
Bug 1256730 - Fail configure when API key files do not exist or are empty.

https://reviewboard.mozilla.org/r/70288/#review68880
Attachment #8779261 - Flags: review?(cmanchester) → review+
Attachment #8779006 - Attachment is obsolete: true
Blocks: 1267425
Comment on attachment 8779262 [details]
Bug 1256730 - Add Mozilla and Google API files to artifact and cross-osx builds.

https://reviewboard.mozilla.org/r/70290/#review108068
Attachment #8779262 - Flags: review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/4b5b1d19453a
Fail configure when API key files do not exist or are empty. r=chmanchester,mikedeboer
https://hg.mozilla.org/integration/autoland/rev/c34a0a8cd144
Add Mozilla and Google API files to artifact and cross-osx builds. r=gps,mikedeboer
Comment on attachment 8779261 [details]
Bug 1256730 - Fail configure when API key files do not exist or are empty.

Approval Request Comment
[Feature/Bug causing the regression]: Prevent bug 1333516 from happening silently
[Is this code covered by automated tests?]: the code is the test
[Has the fix been verified in Nightly?]: it didn't break on autoland
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: the other patch in this bug
[Is the change risky?]: kind of
[Why is the change risky/not risky?]: the risk is that it can turn some builds red, but that only would mean it unveiled breakage like bug 1333516, not that it itself broke something.
[String changes made/needed]: none
Attachment #8779261 - Flags: approval-mozilla-release?
Attachment #8779261 - Flags: approval-mozilla-beta?
Attachment #8779261 - Flags: approval-mozilla-aurora?
Comment on attachment 8779262 [details]
Bug 1256730 - Add Mozilla and Google API files to artifact and cross-osx builds.

Approval Request Comment
[Feature/Bug causing the regression]: This fixes failures uncovered by the other patch
[User impact if declined]: Build failure on artifact and cross-osx builds
[Is this code covered by automated tests?]: code is the test
[Has the fix been verified in Nightly?]: it didn't break on autoland
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: no
[Why is the change risky/not risky?]: it only adds missing files on some taskcluster builds, with the same properties as the same files on TC builds that have them.
[String changes made/needed]: none
Attachment #8779262 - Flags: review?(dustin)
Attachment #8779262 - Flags: approval-mozilla-release?
Attachment #8779262 - Flags: approval-mozilla-beta?
Attachment #8779262 - Flags: approval-mozilla-aurora?
(In reply to Mike Hommey [:glandium] from comment #32)
> Approval Request Comment
> [Feature/Bug causing the regression]: Prevent bug 1333516 from happening
> silently

Side effect: this means is also ensures that bug 1333516 actually works when uplifted.
https://hg.mozilla.org/mozilla-central/rev/4b5b1d19453a
https://hg.mozilla.org/mozilla-central/rev/c34a0a8cd144
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment on attachment 8779261 [details]
Bug 1256730 - Fail configure when API key files do not exist or are empty.

avoid silent API key breakage, aurora53+, beta52+
Attachment #8779261 - Flags: approval-mozilla-beta?
Attachment #8779261 - Flags: approval-mozilla-beta+
Attachment #8779261 - Flags: approval-mozilla-aurora?
Attachment #8779261 - Flags: approval-mozilla-aurora+
Attachment #8779262 - Flags: approval-mozilla-beta?
Attachment #8779262 - Flags: approval-mozilla-beta+
Attachment #8779262 - Flags: approval-mozilla-aurora?
Attachment #8779262 - Flags: approval-mozilla-aurora+
Depends on: 1333921
Comment on attachment 8779261 [details]
Bug 1256730 - Fail configure when API key files do not exist or are empty.

We don't have then plan to have dot release for 51. Rel51-.
Attachment #8779261 - Flags: approval-mozilla-release? → approval-mozilla-release-
Attachment #8779262 - Flags: approval-mozilla-release? → approval-mozilla-release-
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 54 → mozilla54
You need to log in before you can comment on or make changes to this bug.