Summary: | Build ImageDiff with host SDK | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||||||||||||||||||||||||||
Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | ap, commit-queue, dbates, ddkilzer, glenn, lforschler, mitz, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=168939 | ||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||
Bug Blocks: | 168944 | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Jonathan Bedard
2017-02-17 13:12:26 PST
Created attachment 301976 [details]
Patch
Preemptively cq-ing this particular patch since it will likely break our archives. Comment on attachment 301976 [details]
Patch
iOS EWS is red.
ImageDiff is not standalone, so we'd need to build at least WTF with host SDK, if not more. Or to make it standalone, which seems preferable.
Created attachment 302001 [details]
Patch
(In reply to comment #4) > Comment on attachment 301976 [details] > Patch > > iOS EWS is red. > > ImageDiff is not standalone, so we'd need to build at least WTF with host > SDK, if not more. Or to make it standalone, which seems preferable. It's dependent on bmalloc and WTF, the updated patch should fix that issue. I will look into making it a standalone executable, but I'm concerned that this may be a more difficult and less portable solution if we have to do anything like this in the future. I have some basic metrics for compile time. A full build of ImageDiff with no bmalloc or WTF already built takes about 15 seconds. If ImageDiff is built without malloc and WTF dependencies, it takes about a second. It seems that removing the RetainPtr dependency is going to be the best approach here. Created attachment 302551 [details]
Patch
Comment on attachment 302551 [details]
Patch
I think this is the way we want to do this.
This approach makes building ImageDiff part of run-webkit-tests. I suppose this also means we should stop archiving ImageDiff.
Created attachment 302564 [details]
Patch
Attachment 302564 [details] did not pass style-queue:
ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:33: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:34: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:154: Missing space before ( in for( [whitespace/parens] [5]
ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:163: An else should appear on the same line as the preceding } [whitespace/newline] [4]
Total errors found: 4 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #11) > Attachment 302564 [details] did not pass style-queue: > > > ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:33: Alphabetical sorting problem. > [build/include_order] [4] > ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:34: Alphabetical sorting problem. > [build/include_order] [4] > ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:154: Missing space before ( in > for( [whitespace/parens] [5] > ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:163: An else should appear on the > same line as the preceding } [whitespace/newline] [4] > Total errors found: 4 in 5 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. These were all style error present in the original code. The alphabetical sorting issues should not be fixed, although the other style problems should be. Comment on attachment 302564 [details] Patch Attachment 302564 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3181047 New failing tests: media/modern-media-controls/volume-down-support/volume-down-support.html Created attachment 302574 [details]
Archive of layout-test-results from ews115 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 302669 [details]
Patch
Attachment 302669 [details] did not pass style-queue:
ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:33: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:34: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 302669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302669&action=review > Tools/ChangeLog:16 > + (min): Use instead of WTF min. > + (max): Use instead of WTF max. Can we use std::min and std::max? > Tools/ImageDiff/ImageDiff.xcodeproj/project.pbxproj:184 > + MACOSX_DEPLOYMENT_TARGET = 10.12; Hmm. Comment on attachment 302669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302669&action=review > Tools/ImageDiff/mac/ImageDiff.cpp:71 > +template<typename T> inline T min(T var1, T var2) > +{ > + if (var1 < var2) > + return var1; > + return var2; > +} > + > +template<typename T> inline T max(T var1, T var2) > +{ > + if (var1 > var2) > + return var1; > + return var2; > +} Why can't you use std::min/max? > Tools/ImageDiff/mac/ImageDiff.cpp:73 > +static CGImageRef createImageFromStdin(int bytesRemaining) Stdin or StdIn? > Tools/ImageDiff/mac/ImageDiff.cpp:85 > + CGImageRef ret = CGImageCreateWithPNGDataProvider(dataProvider, 0, false, kCGRenderingIntentDefault); We usually call this "result'. Comment on attachment 302669 [details]
Patch
At the moment, this patch would break archives. Preparing a new one which will build ImageDiff with the host and target SDK, but not use the host ImageDiff.
Ok, so, to preserve the sanity of this change, I'm going to split it up. This bug will track building, but not using ImageDiff with the host SDK. Other bugs will track archiving the host-built ImageDiff, making ImageDiff a stand-alone project and using the host-build ImageDiff. A patch which does all of these things changes too much at one time. The incoming patch will look very similar to attachment 20170217154642. Created attachment 302883 [details]
Patch
The patch 302883 causes ImageDiff to build for both the host and the target. Next, ImageDiff will be archived and packaged for the host and the target (bug 168944). Then, the Python scripts will switch to using the ImageDiff built for the host (bug 168945). Then, we will stop archiving and packaging ImageDiff for the target (bug 168946). Lastly, we will make ImageDiff a stand-alone application (bug 168939). Created attachment 302937 [details]
Patch
Created attachment 302980 [details]
Patch
Comment on attachment 302980 [details]
Patch
cq- for the moment. It contains the Mac 32-bit build fix in build-webkit, but not in the Makefiles.
Created attachment 302990 [details]
Patch
Attachment 302990 [details] shouldn't break any builds. This patch has mostly been tested locally, although some of the port-specific Makefile bits are still being locally tested, this will take some time and I will update this bug once those tests are complete.
Created attachment 303067 [details]
Patch
Comment on attachment 303067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303067&action=review > Tools/ImageDiff/Makefile:15 > + $(SCRIPTS_PATH)/build-imagediff --clean --clean => clean Created attachment 305020 [details]
Patch
Created attachment 306096 [details]
Patch
Created attachment 306840 [details]
Patch
Comment on attachment 306840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306840&action=review r=me > Tools/ChangeLog:8 > + ImageDiff should be built and ran with the host SDK, not the target SDK. Typo: ran => run > Tools/Scripts/build-webkit:302 > + my @command = (getcwd() . "/Tools/Scripts/build-imagediff"); I prefer to use File::Spec->catfile(getcwd(), "Tools/Scripts/build-imagediff") in these cases, but making this change doesn't block landing. > Tools/Scripts/webkitdirs.pm:459 > + my @extract = ('--device', '--efl', '--gtk', '--ios', '--platform', '--sdk', '--simulator', '--wincairo', 'SDKROOT', 'ARCHS', "'ARCHS", '"ARCHS'); Do the single/double quotes around ARCHS really survive here after being processed by the shell? Just wondering if the last two items can be removed. > Tools/Scripts/webkitdirs.pm:465 > + if (length($line) >= length($_) && substr($line, 0, length($_)) eq $_ > + && index($line, 'i386') == -1 && index($line, 'x86_64') == -1) { This will break if devices ever existed that used Intel chips but didn't run macOS. Maybe make the subroutine name more Mac-specific: extractNonMacOSHostConfiguration()? > Tools/Scripts/webkitperl/webkitdirs_unittest/extractNonHostConfiguration.pl:1 > +#!/usr/bin/perl -w Nice! Love the unit tests! Created attachment 307000 [details]
Patch
Comment on attachment 307000 [details] Patch Clearing flags on attachment: 307000 Committed r215334: <http://trac.webkit.org/changeset/215334> All reviewed patches have been landed. Closing bug. |