Closed Bug 888132 Opened 7 years ago Closed 7 years ago

MOZ_GOOGLE_API_KEY_FILE should be an include file.

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla25

People

(Reporter: dougt, Assigned: glandium)

Details

Attachments

(2 files, 1 obsolete file)

In order to hide the key from the build logs, we should just #include a file containing a #define.

So, the google key file should basically look like:


#define MOZ_GOOGLE_API_KEY secret_key_asdfasdfasdf
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #768709 - Flags: review?(khuey)
I think if this is pushed to try without catlee's changes, thing will not end well.
Comment on attachment 768709 [details] [diff] [review]
patch v.1

Review of attachment 768709 [details] [diff] [review]:
-----------------------------------------------------------------

Besides the problems in the patch itself, I don't even see what this prevents. Are we displaying the content of mozilla-config.h in the build logs?

::: configure.in
@@ +4341,2 @@
>      MOZ_GOOGLE_API_KEY=no-google-api-key
> +    AC_DEFINE_UNQUOTED(MOZ_GOOGLE_API_KEY, $MOZ_GOOGLE_API_KEY)

Note this puts a useless value in mozilla-config.h.

@@ -4341,3 @@
>      MOZ_GOOGLE_API_KEY=no-google-api-key
> -fi
> -AC_DEFINE_UNQUOTED(MOZ_GOOGLE_API_KEY, $MOZ_GOOGLE_API_KEY)

Note this was putting the key in mozilla-config.h.

::: toolkit/components/urlformatter/Makefile.in
@@ +14,5 @@
>    nsURLFormatter.manifest \
>    $(NULL)
>  
> +ifdef MOZ_GOOGLE_API_KEY_FILE
> +	LOCAL_INCLUDES += MOZ_GOOGLE_API_KEY_FILE

Hint: LOCAL_INCLUDES takes compiler flags.
Hint #2: This is not a variable expansion.
Hint #3: That variable is not AC_SUBSTed in configure.in
Attachment #768709 - Flags: review?(khuey) → review-
> I don't even see what this prevents

Currently we pass the key on the commandline to every compiler invocation.  I don't want it in the build logs.

>  Are we displaying the content of mozilla-config.h in the build logs?

This is a new file.  I don't see the key in my builds.

> Note this puts a useless value in mozilla-config.h.

That right!  we need that because we use this value in the nsURLFormater.js:

 1:08.10 __main__.Error: ('/Volumes/builds/mozilla-central/toolkit/components/urlformatter/nsURLFormatter.js', 106, 'UNDEFINED_VAR', 'MOZ_GOOG\
Attached patch patch v.2Splinter Review
untested...

glandium, is this the general direction you're thinking?
Attachment #768709 - Attachment is obsolete: true
Attachment #768727 - Flags: feedback?(mh+mozilla)
Comment on attachment 768727 [details] [diff] [review]
patch v.2

Review of attachment 768727 [details] [diff] [review]:
-----------------------------------------------------------------

You also need the following in toolkig/components/urlformatter/Makefile.in:

ifdef MOZ_GOOGLE_API_KEY_FILE
DEFINES += -DMOZ_GOOGLE_API_KEY_FILE=$(MOZ_GOOGLE_API_KEY_FILE)
endif

Then the key file will contain:
#define MOZ_GOOGLE_API_KEY_FILE the-key

If you don't want to have to deal with a key file change, and the fact that implementation details end up in it, you can also use an intermediate file and include that file instead.

That is, add something like this in the Makefile.in:
ifdef MOZ_GOOGLE_API_KEY_FILE
google_api_key: $(MOZ_GOOGLE_API_KEY_FILE)
MOZ_GOOGLE_API_KEY = $(shell cat $(MOZ_GOOGLE_API_KEY_FILE))
endif

google_api_key:
        @echo "#define MOZ_GOOGLE_API_KEY $(MOZ_GOOGLE_API_KEY)" > $@

GARBAGE += google_api_key

And just #include google_api_key from nsURLFormatter.js.

::: configure.in
@@ +4335,5 @@
>  # Allow to specify a Google API key file that contains the secret key to be
>  # used for various Google API requests.
>  MOZ_ARG_WITH_STRING(google-api-keyfile,
>  [  --with-google-api-keyfile=file   Use the secret key contained in the given keyfile for Google API requests],
> +  MOZ_GOOGLE_API_KEY_FILE=`$withval`)

Remove the backquotes, or configure is going to execute the key file.
you need to add AC_SUBST(MOZ_GOOGLE_API_KEY_FILE)

::: toolkit/components/urlformatter/nsURLFormatter.js
@@ +15,5 @@
>   *   http[s]://%SERVICE%.mozilla.[com|org]/%LOCALE%/
>   */
>  
> +#ifndef MOZ_GOOGLE_API_KEY_FILE
> +#define MOZ_GOOGLE_API_KEY no-key

no-google-api-key if you want the same non-value as before.

@@ +108,5 @@
>      XPCOMABI:         function() this.ABI,
>      BUILD_TARGET:     function() this.appInfo.OS + "_" + this.ABI,
>      OS_VERSION:       function() this.OSVersion,
>      CHANNEL:          function() UpdateChannel.get(),
> +    GOOGLE_API_KEY:   function() MOZ_GOOGLE_API_KEY,

Just keep that line as it was.
Attachment #768727 - Flags: feedback?(mh+mozilla) → feedback+
Attached patch Another approachSplinter Review
This is another approach that works for me.
Assignee: doug.turner → mh+mozilla
Attachment #768742 - Flags: review?(khuey)
Comment on attachment 768742 [details] [diff] [review]
Another approach

Review of attachment 768742 [details] [diff] [review]:
-----------------------------------------------------------------

This will end up with the key in the build log which is what dougt wants to avoid, no?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> Comment on attachment 768742 [details] [diff] [review]
> Another approach
> 
> Review of attachment 768742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This will end up with the key in the build log which is what dougt wants to
> avoid, no?

How would it? The echo command line is not printed (prefixed with @).
Bah, I was thinking @ was printed.
https://hg.mozilla.org/mozilla-central/rev/e07fc1165da9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
This file was deleted by Ms2ger in bug 887012, which was directly landed on m-c. As a result, it was deleted when inbound got merged to m-c. Reverted and re-landed.
https://hg.mozilla.org/mozilla-central/rev/03207daba2fe
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.