Summary: | [WinCairo] Compile error when environment variable WEBKITLIBRARIESDIR is not defined. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | peavo | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, buildbot, commit-queue, rniwa | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
peavo
2015-04-16 07:04:25 PDT
Created attachment 250920 [details]
Patch
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 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
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? (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. 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. 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. Comment on attachment 250926 [details]
Patch
r=me!
(In reply to comment #11) > Comment on attachment 250926 [details] > Patch > > r=me! Thanks for reviewing :) Comment on attachment 250926 [details] Patch Clearing flags on attachment: 250926 Committed r182891: <http://trac.webkit.org/changeset/182891> All reviewed patches have been landed. Closing bug. |