1256730 - Fail if api key files don't exist

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

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.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug1256730.patch

r+ from glandium in bug 1231320

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: Bug 1256730: fail configure when api keyfiles do not exist;

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

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

(re-requesting review since this has been 5mo since r+)

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8779006 [details]
Bug 1256730: fail configure when api keyfiles do not exist;

https://reviewboard.mozilla.org/r/70070/#review67560

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1256730 - Fail configure when API key files do not exist or are empty.

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

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1256730 - Add Mozilla and Google API files to artifact and cross-osx builds.

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

Comment 7

[Mike Hommey [:glandium]]

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

Comment 8

[Mike Hommey [:glandium]]

It's actually funnier than that, it's missing on buildbot builds too.

Comment 9

[Mike Hommey [:glandium]]

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

Comment 10

[Mike Hommey [:glandium]]

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...

Comment 11

[Mike Hommey [:glandium]]

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

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

Comment 12

[Mike Hommey [:glandium]]

Attachment: Bug 1256730 - Fail configure when API keyfiles do not exist or are empty.

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

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8779261 [details]
Bug 1256730 - Fail configure when API key files do not exist or are empty.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70288/diff/1-2/

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8779262 [details]
Bug 1256730 - Add Mozilla and Google API files to artifact and cross-osx builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70290/diff/1-2/

Comment 15

[Mike de Boer [:mikedeboer]]

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 16

[Mike de Boer [:mikedeboer]]

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!

Comment 17

[Mike de Boer [:mikedeboer]]

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

Comment 18

[Mike Hommey [:glandium]]

Attachment: Bug 1256730 - Add Mozilla and Google API files to artifact builds.

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

Comment 19

[Mike Hommey [:glandium]]

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)

Comment 20

[Dustin J. Mitchell [:dustin] (he/him)]

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!

Comment 21

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8779298 [details]
Bug 1256730 - Add Mozilla and Google API files to artifact builds.

https://reviewboard.mozilla.org/r/70310/#review67598

Comment 22

[Chris Manchester (limited bugmail, email directly)]

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

Comment 23

[Chris Manchester (limited bugmail, email directly)]

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?

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8779261 [details]
Bug 1256730 - Fail configure when API key files do not exist or are empty.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70288/diff/2-3/

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8779262 [details]
Bug 1256730 - Add Mozilla and Google API files to artifact and cross-osx builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70290/diff/2-3/

Comment 26

[Mike Hommey [:glandium]]

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.

Comment 27

[Chris Manchester (limited bugmail, email directly)]

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

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8779261 [details]
Bug 1256730 - Fail configure when API key files do not exist or are empty.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70288/diff/3-4/

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 8779262 [details]
Bug 1256730 - Add Mozilla and Google API files to artifact and cross-osx builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70290/diff/3-4/

Comment 30

[Gregory Szorc [:gps]]

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

Comment 31

[Pulsebot]

Pushed by [e-mail]:
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 32

[Mike Hommey [:glandium]]

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

Comment 33

[Mike Hommey [:glandium]]

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

Comment 34

[Mike Hommey [:glandium]]

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

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b5b1d19453a
https://hg.mozilla.org/mozilla-central/rev/c34a0a8cd144

Comment 36

[Julien Cristau [:jcristau]]

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+

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d68404317ac4
https://hg.mozilla.org/releases/mozilla-aurora/rev/d72b62654730

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/10be69536c44
https://hg.mozilla.org/releases/mozilla-beta/rev/e8e90b0da7c4

Comment 39

[Gerry Chang [:gchang]]

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-.