Fixed file_version_info so that it finds Mac values correctly.

file_version_info was not finding Mac values correctly. Got it now looking in the appropriate bundle.


BUG=NONE
TEST=BUILD

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69728

[dmac, 2010-12-13 18:40:43]

New improved version. I tried to break it up earlier as you saw, but quickly
realized that wasn't going to work as planned.

[dmac, 2010-12-14 17:44:07]

Changed logging over to vlog so it was easier to control the spew.

[Erik does not do reviews, 2010-12-14 19:16:32]

http://codereview.chromium.org/5815001/diff/3001/base/file_version_info_mac.mm
File base/file_version_info_mac.mm (right):

http://codereview.chromium.org/5815001/diff/3001/base/file_version_info_mac.m...
base/file_version_info_mac.mm:21: if (![bundle bundleIdentifier]) {
if mainBundle is bogus, don't we have a big problem?  shouldn't this use
NOTREACHED or even CHECK?

http://codereview.chromium.org/5815001/diff/3001/chrome/common/chrome_version...
File chrome/common/chrome_version_info.cc (right):

http://codereview.chromium.org/5815001/diff/3001/chrome/common/chrome_version...
chrome/common/chrome_version_info.cc:39: return kChromeVersion;
I think you've lost me here.  What are we trying to accomplish with this change
again?  This seems like it's just misleading.  Why can't we fix the underlying
problem that's leading to bogus versions in bundles?

[dmaclach1, 2010-12-14 21:14:51]

I'm not sure how familiar you are with bundling on the Mac, so please
bear with me if I am explaining stuff you know.

A bundle on the Mac is actually a folder containing the executable and
it's resources. One of these resources is an info.plist file which is
a (usually xml) file containing information about the application. The
finder hides this complexity from the user and just displays the
bundle as a double-clickable object which eventually launches the
executable hidden within the bundle. We build the full executable
version of Chromium as a bundle.

Our unit tests that are based on gtest are run from the command line,
and bundling them would cause a significant change to how unit tests
are run, and how all of our projects are built.

The change I am suggesting here picks up the bundle information
correctly if the executable is in a bundle, otherwise it picks up the
hardcoded values which is basically the only way that unbundled
applications can keep this type of data around. All of this was
"failing" silently previously and actually just returning empty
strings. IMHO this is actually a step forward in correctness.

Hopefully this makes some sense. I've added mmentovai on to the review
to see if he has any better suggestions on how to move forward.

Cheers,
Dave

On Tue, Dec 14, 2010 at 11:26 AM, Erik Kay <erikkay@chromium.org> wrote:
> I don't think this is the right fix.  It will mask problems where we don't
> have version information in places where we should.
> Why can't we add proper version information to our tests?
> Erik
>
> On Tue, Dec 14, 2010 at 11:23 AM, David Maclachlan <dmaclach@google.com>
> wrote:
>>
>> Main bundle is only valid if the app is bundled. In almost all cases
>> our unittests are not bundled and have been retrieving bogus
>> information. I have updated the CL description.
>>
>> Sorry if that was not clear.
>>
>> Cheers,
>> Dave
>>
>> On Tue, Dec 14, 2010 at 11:16 AM,  <erikkay@chromium.org> wrote:
>> >
>> >
>> >
http://codereview.chromium.org/5815001/diff/3001/base/file_version_info_mac.mm
>> > File base/file_version_info_mac.mm (right):
>> >
>> >
>> >
http://codereview.chromium.org/5815001/diff/3001/base/file_version_info_mac.m...
>> > base/file_version_info_mac.mm:21: if (![bundle bundleIdentifier]) {
>> > if mainBundle is bogus, don't we have a big problem?  shouldn't this use
>> > NOTREACHED or even CHECK?
>> >
>> >
>> >
http://codereview.chromium.org/5815001/diff/3001/chrome/common/chrome_version...
>> > File chrome/common/chrome_version_info.cc (right):
>> >
>> >
>> >
http://codereview.chromium.org/5815001/diff/3001/chrome/common/chrome_version...
>> > chrome/common/chrome_version_info.cc:39: return kChromeVersion;
>> > I think you've lost me here.  What are we trying to accomplish with this
>> > change again?  This seems like it's just misleading.  Why can't we fix
>> > the underlying problem that's leading to bogus versions in bundles?
>> >
>> > http://codereview.chromium.org/5815001/
>> >
>
>

[Mark Mentovai, 2010-12-14 21:22:33]

Is the real problem here

“my test was failing because it didn’t have a version?”

I don’t want to pervert (by bloat or increased complexity) production
code for the benefit of tests.

Apple’s docs on the matter seem to have vanished, but how about giving
an Info.plist to the “naked” test executable that needs one? You can
link with

-sectcreate __TEXT __info_plist path/to/Info.plist

If you do that, does the test executable have the version information it needs?

[dmac, 2010-12-15 06:35:27]

On 2010/12/14 21:22:33, Mark Mentovai wrote:
> Is the real problem here
> 
> “my test was failing because it didn’t have a version?”
> 
> I don’t want to pervert (by bloat or increased complexity) production
> code for the benefit of tests.
> 
> Apple’s docs on the matter seem to have vanished, but how about giving
> an Info.plist to the “naked” test executable that needs one? You can
> link with
> 
> -sectcreate __TEXT __info_plist path/to/Info.plist
> 
> If you do that, does the test executable have the version information it
needs?

I was unaware that one could do that. I'll give it a shot. Thanks Mark.

[dmac, 2010-12-15 23:52:45]

On 2010/12/15 06:35:27, dmac wrote:
> On 2010/12/14 21:22:33, Mark Mentovai wrote:
> > Is the real problem here
> > 
> > “my test was failing because it didn’t have a version?”
> > 
> > I don’t want to pervert (by bloat or increased complexity) production
> > code for the benefit of tests.
> > 
> > Apple’s docs on the matter seem to have vanished, but how about giving
> > an Info.plist to the “naked” test executable that needs one? You can
> > link with
> > 
> > -sectcreate __TEXT __info_plist path/to/Info.plist
> > 
> > If you do that, does the test executable have the version information it
> needs?
> 
> I was unaware that one could do that. I'll give it a shot. Thanks Mark.

So I was looking into this, and I'm afraid that this particular cure would be
worse than the disease. It appears that CFBundle does still pick up the info in
the __info_plist section, however to get it there would require pretty
significant changes to our build process to get the proper info.plist info in
there. If you take a look at the patch, it does add some conditionals in
chrome_version_info, but removes a lot of other conditionals above
chrome_version_info. The version info that is being returned without the plist
being there is for almost all cases correct and what would come back from the
info.plist. The only place where we would get a difference is for the Last
Build. This would appear to be the easiest way to actually return valid info.

I'd prefer to depend on our own hardcoded values, than a now undocumented
feature of CFBundle.

[Mark Mentovai, 2010-12-16 00:47:01]

OK, I understand.

Can we detect at runtime if we're running inside a test executable and only
return the hard-coded values in that case? Something like "get version, if
version return version, if not bundled return hard-coded, return error?"
(Apologies for the terrible code, I'm on a train.)
On Dec 15, 2010 6:52 PM, <dmaclach@chromium.org> wrote:
> On 2010/12/15 06:35:27, dmac wrote:
>> On 2010/12/14 21:22:33, Mark Mentovai wrote:
>> > Is the real problem here
>> >
>> > “my test was failing because it didn’t have a version?”
>> >
>> > I don’t want to pervert (by bloat or increased complexity) production
>> > code for the benefit of tests.
>> >
>> > Apple’s docs on the matter seem to have vanished, but how about giving
>> > an Info.plist to the “naked” test executable that needs one? You can
>> > link with
>> >
>> > -sectcreate __TEXT __info_plist path/to/Info.plist
>> >
>> > If you do that, does the test executable have the version information
it
>> needs?
>
>> I was unaware that one could do that. I'll give it a shot. Thanks Mark.
>
> So I was looking into this, and I'm afraid that this particular cure would

> be
> worse than the disease. It appears that CFBundle does still pick up the
> info in
> the __info_plist section, however to get it there would require pretty
> significant changes to our build process to get the proper info.plist info

> in
> there. If you take a look at the patch, it does add some conditionals in
> chrome_version_info, but removes a lot of other conditionals above
> chrome_version_info. The version info that is being returned without the
> plist
> being there is for almost all cases correct and what would come back from
> the
> info.plist. The only place where we would get a difference is for the Last
> Build. This would appear to be the easiest way to actually return valid
> info.
>
> I'd prefer to depend on our own hardcoded values, than a now undocumented
> feature of CFBundle.
>
>
>
> http://codereview.chromium.org/5815001/

[dmac, 2010-12-16 21:50:24]

I think I did even better. Now we are working with the override bundle which is
probably what this class should've been using in the first place.

On 2010/12/16 00:47:01, Mark Mentovai wrote:
> OK, I understand.
> 
> Can we detect at runtime if we're running inside a test executable and only
> return the hard-coded values in that case? Something like "get version, if
> version return version, if not bundled return hard-coded, return error?"
> (Apologies for the terrible code, I'm on a train.)
> On Dec 15, 2010 6:52 PM, <mailto:dmaclach@chromium.org> wrote:
> > On 2010/12/15 06:35:27, dmac wrote:
> >> On 2010/12/14 21:22:33, Mark Mentovai wrote:
> >> > Is the real problem here
> >> >
> >> > “my test was failing because it didn’t have a version?”
> >> >
> >> > I don’t want to pervert (by bloat or increased complexity) production
> >> > code for the benefit of tests.
> >> >
> >> > Apple’s docs on the matter seem to have vanished, but how about giving
> >> > an Info.plist to the “naked” test executable that needs one? You can
> >> > link with
> >> >
> >> > -sectcreate __TEXT __info_plist path/to/Info.plist
> >> >
> >> > If you do that, does the test executable have the version information
> it
> >> needs?
> >
> >> I was unaware that one could do that. I'll give it a shot. Thanks Mark.
> >
> > So I was looking into this, and I'm afraid that this particular cure would
> 
> > be
> > worse than the disease. It appears that CFBundle does still pick up the
> > info in
> > the __info_plist section, however to get it there would require pretty
> > significant changes to our build process to get the proper info.plist info
> 
> > in
> > there. If you take a look at the patch, it does add some conditionals in
> > chrome_version_info, but removes a lot of other conditionals above
> > chrome_version_info. The version info that is being returned without the
> > plist
> > being there is for almost all cases correct and what would come back from
> > the
> > info.plist. The only place where we would get a difference is for the Last
> > Build. This would appear to be the easiest way to actually return valid
> > info.
> >
> > I'd prefer to depend on our own hardcoded values, than a now undocumented
> > feature of CFBundle.
> >
> >
> >
> > http://codereview.chromium.org/5815001/

[Mark Mentovai, 2010-12-16 22:11:46]

I only reviewed the Mac bits. They look good, with these comments.

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac.h
File base/file_version_info_mac.h (right):

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac....
base/file_version_info_mac.h:52: std::wstring GetStringValue(CFStringRef name);
Can you name this GetWStringValue, since that’s what it does?

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac.mm
File base/file_version_info_mac.mm (right):

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac....
base/file_version_info_mac.mm:7: #import <Foundation/Foundation.h>
:) Thanks!

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac....
base/file_version_info_mac.mm:19: return
CreateFileVersionInfo(mac_util::MainAppBundlePath());
Ah, that’ll do.

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac....
base/file_version_info_mac.mm:93: return GetStringValue(CFSTR("SVNRevision_"));
Underscore? Eh?

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac....
base/file_version_info_mac.mm:106: const NSString *ns_name =
reinterpret_cast<const NSString *>(name);
Is const really correct here?

[dmac, 2010-12-16 22:20:42]

Thanks Mark. Erik, any other comments?

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac.h
File base/file_version_info_mac.h (right):

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac....
base/file_version_info_mac.h:52: std::wstring GetStringValue(CFStringRef name);
On 2010/12/16 22:11:47, Mark Mentovai wrote:
> Can you name this GetWStringValue, since that’s what it does?

Done.

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac.mm
File base/file_version_info_mac.mm (right):

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac....
base/file_version_info_mac.mm:93: return GetStringValue(CFSTR("SVNRevision_"));
On 2010/12/16 22:11:47, Mark Mentovai wrote:
> Underscore? Eh?

Good catch. Not sure how that got in there...

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac....
base/file_version_info_mac.mm:106: const NSString *ns_name =
reinterpret_cast<const NSString *>(name);
On 2010/12/16 22:11:47, Mark Mentovai wrote:
> Is const really correct here?

it's either that or

NSString *ns_name = const_cast<NSString *>(reinterpret_cast<const NSString
*>(name));

[Mark Mentovai, 2010-12-16 22:39:20]

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac.mm
File base/file_version_info_mac.mm (right):

http://codereview.chromium.org/5815001/diff/17001/base/file_version_info_mac....
base/file_version_info_mac.mm:106: const NSString *ns_name =
reinterpret_cast<const NSString *>(name);
You can use a C-style cast in preference to nesting C++-style ones.

I’m concerned that clang might not like |const NSString*|, but if it works, then
I have no real objection.

[Mark Mentovai, 2010-12-16 22:44:00]

Fix the change description, since you’re not using compiled-in values.

[dmac, 2010-12-20 18:24:18]

Mark/Erik

Could you take a quick look at this significantly slimmed down patch?

[Mark Mentovai, 2010-12-20 18:38:59]

LGTM

[Erik does not do reviews, 2010-12-20 18:42:29]

LGTM