Closed
Bug 888132
Opened 11 years ago
Closed 11 years ago
MOZ_GOOGLE_API_KEY_FILE should be an include file.
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla25
People
(Reporter: dougt, Assigned: glandium)
Details
Attachments
(2 files, 1 obsolete file)
2.95 KB,
patch
|
glandium
:
feedback+
|
Details | Diff | Splinter Review |
1.76 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
Attachment #768709 -
Flags: review?(khuey)
Reporter | ||
Comment 2•11 years ago
|
||
I think if this is pushed to try without catlee's changes, thing will not end well.
Assignee | ||
Comment 3•11 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•11 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•11 years ago
|
||
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•11 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•11 years ago
|
||
This is another approach that works for me.
Assignee | ||
Updated•11 years ago
|
Assignee: doug.turner → mh+mozilla
Assignee | ||
Updated•11 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•11 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.
Attachment #768742 -
Flags: review?(khuey) → review+
Reporter | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e07fc1165da9
Comment 12•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e07fc1165da9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 13•11 years ago
|
||
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•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•