679359 - Autodetect WIN32_REDIST_DIR

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

Once we get rid of old style jemalloc, all builds will require the CRT to work on non development environments, and thus a properly set WIN32_REDIST_DIR. We can make that easier if we can autodetect it.

We could already do that before getting rid of old style jemalloc, but it doesn't make much sense to add the adequate tests to remove them then.

Comment 1

[Benjamin Smedberg]

I do not think that we should automatically do this. We didn't automatically do it to begin with because the MS DLLs are not free software and you have to follow the redistribution agreement. We should require people to pass a flag to acknowledge that they understand what they are getting into, technically and legally.

Comment 2

[Mike Hommey [:glandium]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> I do not think that we should automatically do this. We didn't automatically
> do it to begin with because the MS DLLs are not free software and you have
> to follow the redistribution agreement.

Don't you already have to do that if you link with MSVC (which includes static bits of the CRT)?

Comment 3

[(not currently active) Ted Mielczarek]

We could simplify this to "--enable-crt-redist" or something.

Comment 4

[Justin Wood (:Callek)]

IMO this is twofold,

First WIN32_REDIST_DIR is not required to build, but is necessary if you are distributing the build. That said, if your users locally install the necessary MSVC dll's etc. then you don't necessary have to ship the dll's either.

That said, I also feel this should be a factor of MOZBUILD to locate, since we already do the magic for the MSVC setting in there. We should also only USE the setting if we pass a flag to configure, imo.

So MozBuild will find/set MOZ_WIN32_REDIST_DIR, configure will see that and use it if --enable-crt-redist is passed.

Does all that sound accurate/a good plan?

Comment 5

[Benjamin Smedberg]

Yes, I support automagically locating the directory.

No, statically linking the CRT does not trigger the same license restrictions as the dynamic CRT does. It's not a huge deal, but people should be aware of it before using it.

Comment 6

[Mike Hommey [:glandium]]

I'll leave the MozBuild fiddling to Kyle, then, since he's been playing with it recently.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Autodetect WIN32_REDIST_DIR

Just for the record, this is the configure change I was working on. Only attaching here because it contains details about the REDIST_DIR path differences between versions of MSVC and between architectures.

Comment 8

[Masatoshi Kimura [:emk]]

We already autodetect a location of d3dx9_*.dll and D3DCompiler_*.dll
and MOZ_ANGLE is enabled by default.
Doesn't it cause a problem?

Comment 9

[Mike Hommey [:glandium]]

(In reply to Masatoshi Kimura [:emk] from comment #8)
> We already autodetect a location of d3dx9_*.dll and D3DCompiler_*.dll
> and MOZ_ANGLE is enabled by default.
> Doesn't it cause a problem?

It probably causes the very same problem.

Comment 10

[Rain [:rain1] (inactive)]

Note that once bug 515374 lands and package-manifest warnings are made fatal for an app, any attempt to create a package (make -C $app/installer) without setting WIN32_REDIST_DIR will fail. I think this is the right thing to do, though.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to Siddharth Agarwal [:sid0] from comment #10)

> Note that once bug 515374 lands and package-manifest warnings are made fatal
> for an app, any attempt to create a package (make -C $app/installer) without

(After SeaMonkey actually hit this case...)
I filed Firefox bug 735810.

> setting WIN32_REDIST_DIR will fail. I think this is the right thing to do,
> though.

It shouldn't fail, as long as these dlls are not actually required (in all cases).

Comment 12

[Rain [:rain1] (inactive)]

(In reply to Serge Gautherie (:sgautherie) from comment #11)
> It shouldn't fail, as long as these dlls are not actually required (in all
> cases).

They are required in all reasonable cases. We actually shipped a broken Thunderbird 9.0 because we didn't catch this in time. A warning isn't good enough: we need red on tinderbox.

Comment 13

[Mike Hommey [:glandium]]

You need red on tinderbox, but you don't need red when doing make package locally

Comment 14

[Mark Banner (:standard8)]

How about only failing when --enable-official-branding is set? I don't know how many devs use it, but this would at least fail with builds on beta - in time to catch it before release, but shouldn't affect the majority of devs.

Comment 15

[(not currently active) Ted Mielczarek]

I was going to suggest keying on MOZILLA_OFFICIAL. Either of those would probably work.

Comment 16

[Rain [:rain1] (inactive)]

Sure, makes sense.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Can this be moved forward? It's a footgun for anyone attempting to responsibly test on multiple machines/VMs.

Comment 19

[Mike Hommey [:glandium]]

Attachment: Bug 679359 - Autodetect WIN32_REDIST_DIR.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/551ad8c66bc6
Autodetect WIN32_REDIST_DIR. r=firefox-build-system-reviewers,sergesanspaille,nalexander

Comment 21

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/551ad8c66bc6