Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
809343008:
Add files for libexif GN target. (Closed)
|
Created:
5 years, 11 months ago by Slava Chigrin Modified:
5 years, 3 months ago CC:
chromium-reviews, brucedawson Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd files for libexif GN target.
Committed: https://crrev.com/ea5d3521a1b31b02c1b556647e0f35a7a746f828
Cr-Commit-Position: refs/heads/master@{#311654}
Patch Set 1 #
Total comments: 4
Patch Set 2 : TODO comments added. #
Total comments: 7
Patch Set 3 : Comments updated. #
Total comments: 2
Messages
Total messages: 27 (9 generated)
vchigrin@yandex-team.ru changed reviewers: + thestig@chromium.org, vandebo@chromium.org
https://codereview.chromium.org/809343008/diff/1/third_party/libexif/BUILD.gn File third_party/libexif/BUILD.gn (right): https://codereview.chromium.org/809343008/diff/1/third_party/libexif/BUILD.gn... third_party/libexif/BUILD.gn:41: #TODO(GYP): Additional options for non-Windows platforms. At present I am focused on making GN work on Windows, so I am not familiar with proper way of converting mac-related stuff in GYP. In particular there some breakpad-related things done there. Hope, you'll allow commit present solution and fix non-Windows issues later. https://codereview.chromium.org/809343008/diff/1/third_party/libexif/BUILD.gn... third_party/libexif/BUILD.gn:45: # This seems like a hack, but this is what WebKit Win does. Taken from GYP - see https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libexi...
I don't think anyone is trying to use MSVC + /analyze + GN at the moment, but there's a msvs_settings block in the .gyp version that has been omitted. Maybe at least make a note of that with a TODO? +brucedawson FYI. We also forgot to add the same block to third_party/yasm/BUILD.gn in https://codereview.chromium.org/657883003 https://codereview.chromium.org/809343008/diff/1/third_party/libexif/BUILD.gn File third_party/libexif/BUILD.gn (right): https://codereview.chromium.org/809343008/diff/1/third_party/libexif/BUILD.gn... third_party/libexif/BUILD.gn:6: if (!is_linux || is_chromeos) { Don't you need to handle the linux case as well? Add a TODO at least?
Patchset #2 (id:20001) has been deleted
Thank you for review, uploaded new version. https://codereview.chromium.org/809343008/diff/1/third_party/libexif/BUILD.gn File third_party/libexif/BUILD.gn (right): https://codereview.chromium.org/809343008/diff/1/third_party/libexif/BUILD.gn... third_party/libexif/BUILD.gn:6: if (!is_linux || is_chromeos) { On 2015/01/14 23:17:41, Lei Zhang wrote: > Don't you need to handle the linux case as well? Add a TODO at least? Done. Although I thought that chromium already compiles with GN on Linux, so that issue already solved in some way.
libexif works on Linux because it got lucky. If you don't mind, just add "TODO(thestig): Properly support building on Linux" and I'll take care of it later. https://codereview.chromium.org/809343008/diff/40001/third_party/libexif/BUIL... File third_party/libexif/BUILD.gn (right): https://codereview.chromium.org/809343008/diff/40001/third_party/libexif/BUIL... third_party/libexif/BUILD.gn:59: #TODO(GYP): Remove /analyze, when it will be used in GN. Remove -> Add?
https://codereview.chromium.org/809343008/diff/40001/third_party/libexif/BUIL... File third_party/libexif/BUILD.gn (right): https://codereview.chromium.org/809343008/diff/40001/third_party/libexif/BUIL... third_party/libexif/BUILD.gn:8: # TODO(GYP) Add Linux support, if neccessary. ... that is, you can put my username here. I'll take care of it. BTW, there's a typo: "necessary" https://codereview.chromium.org/809343008/diff/40001/third_party/libexif/BUIL... third_party/libexif/BUILD.gn:43: #TODO(GYP): Additional options for non-Windows platforms. nit: space after the '#', ditto below.
Patchset #3 (id:60001) has been deleted
Also, do you plan on including the libexif GN file in chrome/utility/BUILD.gn? There's currently an entry there that's commented out.
Thank you for review! Yes, I am planning to enable that soon. I have several other small fixes in GN files in chrome/ directory, and plan to upload them in one CL in nearest time, to avoid spamming chrome/ owners with dozens tiny CLs. Hope this is acceptable. https://codereview.chromium.org/809343008/diff/40001/third_party/libexif/BUIL... File third_party/libexif/BUILD.gn (right): https://codereview.chromium.org/809343008/diff/40001/third_party/libexif/BUIL... third_party/libexif/BUILD.gn:8: # TODO(GYP) Add Linux support, if neccessary. On 2015/01/15 08:30:27, Lei Zhang wrote: > ... that is, you can put my username here. I'll take care of it. BTW, there's a > typo: "necessary" Done. https://codereview.chromium.org/809343008/diff/40001/third_party/libexif/BUIL... third_party/libexif/BUILD.gn:43: #TODO(GYP): Additional options for non-Windows platforms. On 2015/01/15 08:30:26, Lei Zhang wrote: > nit: space after the '#', ditto below. Done. https://codereview.chromium.org/809343008/diff/40001/third_party/libexif/BUIL... third_party/libexif/BUILD.gn:59: #TODO(GYP): Remove /analyze, when it will be used in GN. On 2015/01/15 08:24:11, Lei Zhang wrote: > Remove -> Add? In GYP this switch in "AdditionalOptions!" (note exclamation mark) list. And also comment before says "...Therefore, /analyze is disabled for this project...." thats why I thought that this compiler option is being removed there. Thats why I decided to change comment to more neutral "handle" word, to avoid confusion. If I something misunderstood, I can change it to "Add", as you asked.
lgtm https://codereview.chromium.org/809343008/diff/40001/third_party/libexif/BUIL... File third_party/libexif/BUILD.gn (right): https://codereview.chromium.org/809343008/diff/40001/third_party/libexif/BUIL... third_party/libexif/BUILD.gn:59: #TODO(GYP): Remove /analyze, when it will be used in GN. On 2015/01/15 08:42:48, Slava Chigrin wrote: > On 2015/01/15 08:24:11, Lei Zhang wrote: > > Remove -> Add? > > In GYP this switch in "AdditionalOptions!" (note exclamation mark) list. And > also comment before says "...Therefore, /analyze is disabled for this > project...." > thats why I thought that this compiler option is being removed there. > > Thats why I decided to change comment to more neutral "handle" word, to avoid > confusion. If I something misunderstood, I can change it to "Add", as you asked. Right, sorry for the confusion. It's getting late here...
The CQ bit was checked by vchigrin@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/809343008/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by vchigrin@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/809343008/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by vchigrin@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/809343008/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ea5d3521a1b31b02c1b556647e0f35a7a746f828 Cr-Commit-Position: refs/heads/master@{#311654}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/809343008/diff/80001/third_party/libexif/BUIL... File third_party/libexif/BUILD.gn (right): https://codereview.chromium.org/809343008/diff/80001/third_party/libexif/BUIL... third_party/libexif/BUILD.gn:13: static_library("libexif") { doesn't this have to be a dll? image_metadata_extractor.cc is trying to load libexif.dll...
Message was sent while issue was closed.
https://codereview.chromium.org/809343008/diff/80001/third_party/libexif/BUIL... File third_party/libexif/BUILD.gn (right): https://codereview.chromium.org/809343008/diff/80001/third_party/libexif/BUIL... third_party/libexif/BUILD.gn:13: static_library("libexif") { On 2015/09/02 22:54:49, Nico (vacation Thu Sep 2) wrote: > doesn't this have to be a dll? image_metadata_extractor.cc is trying to load > libexif.dll... Yes, my bad. In GYP it defined ad "loadable_module", I will make fix soon. Sorry for this mistake...
Message was sent while issue was closed.
On 2015/09/03 19:28:18, Slava Chigrin wrote: > https://codereview.chromium.org/809343008/diff/80001/third_party/libexif/BUIL... > File third_party/libexif/BUILD.gn (right): > > https://codereview.chromium.org/809343008/diff/80001/third_party/libexif/BUIL... > third_party/libexif/BUILD.gn:13: static_library("libexif") { > On 2015/09/02 22:54:49, Nico (vacation Thu Sep 2) wrote: > > doesn't this have to be a dll? image_metadata_extractor.cc is trying to load > > libexif.dll... > > Yes, my bad. In GYP it defined ad "loadable_module", I will make fix soon. Sorry > for this mistake... Any luck? |
