Bug 143828 – [WinCairo] Compile error when environment variable WEBKITLIBRARIESDIR is not defined.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

Python throws an exception when calling os.environ['VAR'] and VAR is not defined.

Created attachment 250920 [details]
Patch

[Build Bot]

Comment on attachment 250920 [details]
Patch

Attachment 250920 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4623910377095168

New failing tests:
compositing/scrolling/touch-scroll-to-clip.html

[Build Bot]

Created attachment 250922 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Brent Fulgham]

It's hard to see how this change could have caused that test failure!

I'm not sure what we gain with this change. We need WEBKIT_LIBRARIES to be defined, because the Visual Studio projects use it to find the property sheets (e.g., 'common.props', 'debug.props') used to control much of the build.

If we do not define WEBKIT_LIBRARIES, I would expect build failures simply because the visual studio project files would not load properly.

Are you encountering this in conjunction with a CMake-based build? I could see how that might not need the WEBKIT_LIBRARIES environment variable.

(In reply to comment #4)
> It's hard to see how this change could have caused that test failure!
> 
> I'm not sure what we gain with this change. We need WEBKIT_LIBRARIES to be
> defined, because the Visual Studio projects use it to find the property
> sheets (e.g., 'common.props', 'debug.props') used to control much of the
> build.
> 
> If we do not define WEBKIT_LIBRARIES, I would expect build failures simply
> because the visual studio project files would not load properly.
> 
> Are you encountering this in conjunction with a CMake-based build? I could
> see how that might not need the WEBKIT_LIBRARIES environment variable.

Yes, the test failure is strange :)

The problem is not WEBKIT_LIBRARIES (which I have defined), but WEBKITLIBRARIESDIR. I can define WEBKITLIBRARIESDIR, but maybe we don't need both?

[Brent Fulgham]

(In reply to comment #5)
> (In reply to comment #4)
> > It's hard to see how this change could have caused that test failure!
> > 
> > I'm not sure what we gain with this change. We need WEBKIT_LIBRARIES to be
> > defined, because the Visual Studio projects use it to find the property
> > sheets (e.g., 'common.props', 'debug.props') used to control much of the
> > build.
> > 
> > If we do not define WEBKIT_LIBRARIES, I would expect build failures simply
> > because the visual studio project files would not load properly.
> > 
> > Are you encountering this in conjunction with a CMake-based build? I could
> > see how that might not need the WEBKIT_LIBRARIES environment variable.
> 
> Yes, the test failure is strange :)
> 
> The problem is not WEBKIT_LIBRARIES (which I have defined), but
> WEBKITLIBRARIESDIR. I can define WEBKITLIBRARIESDIR, but maybe we don't need
> both?

Oh! I misunderstood.

We should not need both. IIRC, WEBKITLIBRARIESDIR was the 'old' name. We (rfong) switched to the newer names back in 2013. We should probably just get rid of the WEBKITLIBRARIESDIR environment variable entirely.

[Brent Fulgham]

Comment on attachment 250920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250920&action=review

r=me if you get rid of the WEBKITLIBRARIESDIR portion of this change.

> Source/WebCore/AVFoundationSupport.py:34
> +    return os.path.isfile(os.environ.get('WEBKIT_LIBRARIES', '') + relativePath) or os.path.isfile(os.environ.get('WEBKITLIBRARIESDIR', '') + relativePath)

Oh! WEBKITLIBRARIESDIR is old stuff we don't use anymore (it was replaced by WEBKIT_LIBRARIES). Let's just get rid of the "WEBKITLIBRARIESDIR" part of this entirely. We don't need it, and we certainly do not want to re-introduce this old variable.

[Brent Fulgham]

Comment on attachment 250920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250920&action=review

>> Source/WebCore/AVFoundationSupport.py:34
>> +    return os.path.isfile(os.environ.get('WEBKIT_LIBRARIES', '') + relativePath) or os.path.isfile(os.environ.get('WEBKITLIBRARIESDIR', '') + relativePath)
> 
> Oh! WEBKITLIBRARIESDIR is old stuff we don't use anymore (it was replaced by WEBKIT_LIBRARIES). Let's just get rid of the "WEBKITLIBRARIESDIR" part of this entirely. We don't need it, and we certainly do not want to re-introduce this old variable.

... it might be good to let this throw if WEBKIT_LIBRARIES doesn't exist, or perhaps generate a meaningful error here. The build (on Windows without CMake) *WILL* fail if WEBKIT_LIBRARIES is not defined, perhaps in confusing and mysterious ways. It would help developers identify the problem quickly if they could see this error that clearly shows the missing environment variable.

Created attachment 250926 [details]
Patch

Ok, thanks :)

I didn't see your last comments before uploading the patch, the latest patch will throw if WEBKIT_LIBRARIES does not exist.

[Brent Fulgham]

Comment on attachment 250926 [details]
Patch

r=me!

(In reply to comment #11)
> Comment on attachment 250926 [details]
> Patch
> 
> r=me!

Thanks for reviewing :)

[WebKit Commit Bot]

Comment on attachment 250926 [details]
Patch

Clearing flags on attachment: 250926

Committed r182891: <http://trac.webkit.org/changeset/182891>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.