Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1734423002:
[qcms] Fix build issue of qcms_tests for MIPS Linux (Closed)
Description[qcms] Fix build issue of qcms_tests for MIPS Linux
Building all target on MIPS will trigger issue with qcms_tests:
../mipsel-linux-gnu/Scrt1.o: In function `__start':
(.text+0x1c): undefined reference to `main'
clang: error: linker command failed with exit code 1
This change will make possible to build qcms_tests (without testing for
SSE2) for non ia32/x64 platforms.
BUG=130022, 590227
TEST=ninja -C out/Release all, qcms_tests -a -i sRGB.icc -o AdobeRGB1998.icc -s
Committed: https://crrev.com/e7e0e4405b3882356c581d838e1f9bfc730572d5
Cr-Commit-Position: refs/heads/master@{#380124}
Patch Set 1 #Patch Set 2 : Including third_party/WebKit/LayoutTests/whitespace.txt #
Total comments: 4
Patch Set 3 : Added BUILD.gn and addressed comments. #
Messages
Total messages: 32 (10 generated)
milko.leporis@imgtec.com changed reviewers: + noel@chromium.org
Build for MIPS was broken by https://codereview.chromium.org/1609643003 Please take a look and advise if it even makes sense to build and run qcms_tests on platforms other than ia32/x64.
Description was changed from ========== [MIPS] Fix build issue of qcms_tests for MIPS Linux Building all target on MIPS will trigger issue with qcms_tests: ../mipsel-linux-gnu/Scrt1.o: In function `__start': (.text+0x1c): undefined reference to `main' clang: error: linker command failed with exit code 1 This change will make possible to build qcms_tests (without test for SSE2) for non ia32/x64 platforms. BUG=130022, 590227 TEST=ninja -C out/Release all, qcms_tests -a -i sRGB.icc -o AdobeRGB1998.icc -s ========== to ========== [qcms] Fix build issue of qcms_tests for MIPS Linux Building all target on MIPS will trigger issue with qcms_tests: ../mipsel-linux-gnu/Scrt1.o: In function `__start': (.text+0x1c): undefined reference to `main' clang: error: linker command failed with exit code 1 This change will make possible to build qcms_tests (without testing for SSE2) for non ia32/x64 platforms. BUG=130022, 590227 TEST=ninja -C out/Release all, qcms_tests -a -i sRGB.icc -o AdobeRGB1998.icc -s ==========
On 2016/02/26 16:26:09, lmilko wrote: I was out of the office last week, soory for the delayed reply. > Build for MIPS was broken by https://codereview.chromium.org/1609643003 I see. There's no MIPS bot that we can check against (at least, not in the chromium / blink infrastructure). > Please take a look and advise if it even makes sense to build and run qcms_tests > on platforms other than ia32/x64 You would run these tests when making changes to, or using, QCMS on your platform. If you don't plan to use QCMS on a MIPS platform, then there's no need to build qcms_tests, right?
On 2016/02/26 16:26:09, lmilko wrote: I was out of the office last week, sorry for the delayed reply. > Build for MIPS was broken by https://codereview.chromium.org/1609643003 I see. There's no MIPS bot that we can check against (at least, not in the chromium / blink infrastructure). > Please take a look and advise if it even makes sense to build and run qcms_tests > on platforms other than ia32/x64 You would run these tests when making changes to, or using, QCMS on your platform. If you don't plan to use QCMS on a MIPS platform, then there's no need to build qcms_tests, right?
On 2016/03/03 04:06:41, noel gordon wrote: > On 2016/02/26 16:26:09, lmilko wrote: > > I was out of the office last week, sorry for the delayed reply. > > > Build for MIPS was broken by https://codereview.chromium.org/1609643003 > > I see. There's no MIPS bot that we can check against (at least, not in the > chromium / blink infrastructure). > > > Please take a look and advise if it even makes sense to build and run > qcms_tests > > on platforms other than ia32/x64 > > You would run these tests when making changes to, or using, QCMS on your > platform. If you don't plan to use QCMS on a MIPS platform, then there's no > need to build qcms_tests, right? Right, since qcms lib is being used in Chromium for MIPS Linux, then we need the tests. This change makes qcms_tests compile for MIPS and other non ia32/x64 arches. Please review.
On 2016/03/03 13:13:57, lmilko wrote: > Right, since qcms lib is being used in Chromium for MIPS Linux, > then we need the tests. I see. Note: with no acceleration support (like SSE), I expect color correction would be very slow on MIPS Linux. > This change makes qcms_tests compile for MIPS and other non ia32/x64 arches. > > Please review. Why is there no CQ dry run here? Without it, I can't see if the chromium and webkit tests are passing. Make a white space change to third_party/WebKit/LayoutTests/whitespace.txt and add that file to this CL (this to force the layout tests to be run CQ-ing your patch). +radu, +robert, they should also review this patch.
noel@chromium.org changed reviewers: + radu.velea@intel.com, robert.bradford@intel.com
Could you also edit Build.gn? https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/qcms.gyp File third_party/qcms/qcms.gyp (right): https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/qcms.g... third_party/qcms/qcms.gyp:93: 'src/tests/qcms_test_tetra_clut_rgba.c', Can we maybe have the ifdefs in qcms_test_tetra_clut_rgba to split SSE from other vector implementations (for now there are none so maybe a dummy function would do)? https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/src/te... File third_party/qcms/src/tests/qcms_test_main.c (right): https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_main.c:15: #ifdef SSE2_ENABLE These look like too many ifdef's to me. Maybe we should have another target for the tests: build a shared library and then query it at runtime for whatever symbols we want to test? Or maybe a dummy test for qcms_test_tetra_clut_rgba on non-sse platforms?
Edited BUILD.gn to build qcms_tests on non x86/x64 platforms. Added dummy test function to be used instead of SSE2 one on non x86/x64 platforms. https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/qcms.gyp File third_party/qcms/qcms.gyp (right): https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/qcms.g... third_party/qcms/qcms.gyp:93: 'src/tests/qcms_test_tetra_clut_rgba.c', On 2016/03/04 13:33:08, radu.velea wrote: > Can we maybe have the ifdefs in qcms_test_tetra_clut_rgba to split SSE from > other vector implementations (for now there are none so maybe a dummy function > would do)? Done. https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/src/te... File third_party/qcms/src/tests/qcms_test_main.c (right): https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_main.c:15: #ifdef SSE2_ENABLE On 2016/03/04 13:33:08, radu.velea wrote: > These look like too many ifdef's to me. Maybe we should have another target for > the tests: build a shared library and then query it at runtime for whatever > symbols we want to test? > > Or maybe a dummy test for qcms_test_tetra_clut_rgba on non-sse platforms? Done.
On 2016/03/08 13:21:59, lmilko wrote: > Edited BUILD.gn to build qcms_tests on non x86/x64 platforms. > Added dummy test function to be used instead of SSE2 one on non x86/x64 > platforms. > > https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/qcms.gyp > File third_party/qcms/qcms.gyp (right): > > https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/qcms.g... > third_party/qcms/qcms.gyp:93: 'src/tests/qcms_test_tetra_clut_rgba.c', > On 2016/03/04 13:33:08, radu.velea wrote: > > Can we maybe have the ifdefs in qcms_test_tetra_clut_rgba to split SSE from > > other vector implementations (for now there are none so maybe a dummy function > > would do)? > > Done. > > https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/src/te... > File third_party/qcms/src/tests/qcms_test_main.c (right): > > https://codereview.chromium.org/1734423002/diff/20001/third_party/qcms/src/te... > third_party/qcms/src/tests/qcms_test_main.c:15: #ifdef SSE2_ENABLE > On 2016/03/04 13:33:08, radu.velea wrote: > > These look like too many ifdef's to me. Maybe we should have another target > for > > the tests: build a shared library and then query it at runtime for whatever > > symbols we want to test? > > > > Or maybe a dummy test for qcms_test_tetra_clut_rgba on non-sse platforms? > > Done. lgtm
The CQ bit was checked by milko.leporis@imgtec.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1734423002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1734423002/40001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
rs+ LGTM.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1734423002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1734423002/40001
CQ+ dry-run.
CQ+ dry-run.
CQ+ dry-run.
CQ+ dry-run.
CQ+ dry-run.
CQ+ dry-run.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1734423002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1734423002/40001
Message was sent while issue was closed.
Description was changed from ========== [qcms] Fix build issue of qcms_tests for MIPS Linux Building all target on MIPS will trigger issue with qcms_tests: ../mipsel-linux-gnu/Scrt1.o: In function `__start': (.text+0x1c): undefined reference to `main' clang: error: linker command failed with exit code 1 This change will make possible to build qcms_tests (without testing for SSE2) for non ia32/x64 platforms. BUG=130022, 590227 TEST=ninja -C out/Release all, qcms_tests -a -i sRGB.icc -o AdobeRGB1998.icc -s ========== to ========== [qcms] Fix build issue of qcms_tests for MIPS Linux Building all target on MIPS will trigger issue with qcms_tests: ../mipsel-linux-gnu/Scrt1.o: In function `__start': (.text+0x1c): undefined reference to `main' clang: error: linker command failed with exit code 1 This change will make possible to build qcms_tests (without testing for SSE2) for non ia32/x64 platforms. BUG=130022, 590227 TEST=ninja -C out/Release all, qcms_tests -a -i sRGB.icc -o AdobeRGB1998.icc -s ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [qcms] Fix build issue of qcms_tests for MIPS Linux Building all target on MIPS will trigger issue with qcms_tests: ../mipsel-linux-gnu/Scrt1.o: In function `__start': (.text+0x1c): undefined reference to `main' clang: error: linker command failed with exit code 1 This change will make possible to build qcms_tests (without testing for SSE2) for non ia32/x64 platforms. BUG=130022, 590227 TEST=ninja -C out/Release all, qcms_tests -a -i sRGB.icc -o AdobeRGB1998.icc -s ========== to ========== [qcms] Fix build issue of qcms_tests for MIPS Linux Building all target on MIPS will trigger issue with qcms_tests: ../mipsel-linux-gnu/Scrt1.o: In function `__start': (.text+0x1c): undefined reference to `main' clang: error: linker command failed with exit code 1 This change will make possible to build qcms_tests (without testing for SSE2) for non ia32/x64 platforms. BUG=130022, 590227 TEST=ninja -C out/Release all, qcms_tests -a -i sRGB.icc -o AdobeRGB1998.icc -s Committed: https://crrev.com/e7e0e4405b3882356c581d838e1f9bfc730572d5 Cr-Commit-Position: refs/heads/master@{#380124} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e7e0e4405b3882356c581d838e1f9bfc730572d5 Cr-Commit-Position: refs/heads/master@{#380124} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
