MOZ_GOOGLE_API_KEY_FILE should be an include file.

RESOLVED FIXED in mozilla25

Status

Firefox Build System
General
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: dougt, Assigned: glandium)

Tracking

unspecified
mozilla25
x86
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
Created attachment 768709 [details] [diff] [review]
patch v.1
Attachment #768709 - Flags: review?(khuey)
(Reporter)

Comment 2

5 years ago
I think if this is pushed to try without catlee's changes, thing will not end well.
(Assignee)

Comment 3

5 years ago
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-
(Reporter)

Comment 4

5 years ago
> 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\
(Reporter)

Comment 5

5 years ago
Created attachment 768727 [details] [diff] [review]
patch v.2

untested...

glandium, is this the general direction you're thinking?
Attachment #768709 - Attachment is obsolete: true
Attachment #768727 - Flags: feedback?(mh+mozilla)
(Assignee)

Comment 6

5 years ago
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+
(Assignee)

Comment 7

5 years ago
Created attachment 768742 [details] [diff] [review]
Another approach

This is another approach that works for me.
(Assignee)

Updated

5 years ago
Assignee: doug.turner → mh+mozilla
(Assignee)

Updated

5 years ago
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?
(Assignee)

Comment 9

5 years ago
(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
Last Resolved: 5 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

Updated

3 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.