Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1900823002:
Use lkgr revision of Clang for PDFium (Closed)
DescriptionUse lkgr revision of Clang for PDFium
Recent change of Clang (https://codereview.chromium.org/1879753002)
invokes GetVSVersion() which imports vs_toolchain which PDFium has
not used yet. This breaks our bots. Now we pinned Clang to last good
version.
We can either change back to use Tot Clang after we have toolchain
utility, or we can roll Clang periodically.
Committed: https://pdfium.googlesource.com/pdfium/+/b34d72b1045aafa420f6749b098e0ae6d9ce18b9
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #Messages
Total messages: 16 (7 generated)
weili@chromium.org changed reviewers: + thestig@chromium.org, tsepez@chromium.org
Original CL owner is not reachable. Another Clang member is not familiar with our code, and suggested us to add vs_toolchain. I think this is a good alternative. WDYT?
lgtm https://codereview.chromium.org/1900823002/diff/1/DEPS File DEPS (right): https://codereview.chromium.org/1900823002/diff/1/DEPS#newcode16 DEPS:16: 'clang_revision': '87058e09f9c547eb5d00cb8ca666c6aec203a117', nit alphabetize https://codereview.chromium.org/1900823002/diff/1/DEPS#newcode39 DEPS:39: "tools/clang": nit: here too
Patchset #2 (id:20001) has been deleted
thanks https://codereview.chromium.org/1900823002/diff/1/DEPS File DEPS (right): https://codereview.chromium.org/1900823002/diff/1/DEPS#newcode16 DEPS:16: 'clang_revision': '87058e09f9c547eb5d00cb8ca666c6aec203a117', On 2016/04/18 18:51:46, Tom Sepez wrote: > nit alphabetize Done. https://codereview.chromium.org/1900823002/diff/1/DEPS#newcode39 DEPS:39: "tools/clang": On 2016/04/18 18:51:46, Tom Sepez wrote: > nit: here too This one is already in order.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by weili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1900823002/#ps60001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1900823002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1900823002/60001
Message was sent while issue was closed.
Description was changed from ========== Use lkgr revision of Clang for PDFium Recent change of Clang (https://codereview.chromium.org/1879753002) invokes GetVSVersion() which imports vs_toolchain which PDFium has not used yet. This breaks our bots. Now we pinned Clang to last good version. We can either change back to use Tot Clang after we have toolchain utility, or we can roll Clang periodically. ========== to ========== Use lkgr revision of Clang for PDFium Recent change of Clang (https://codereview.chromium.org/1879753002) invokes GetVSVersion() which imports vs_toolchain which PDFium has not used yet. This breaks our bots. Now we pinned Clang to last good version. We can either change back to use Tot Clang after we have toolchain utility, or we can roll Clang periodically. Committed: https://pdfium.googlesource.com/pdfium/+/b34d72b1045aafa420f6749b098e0ae6d9ce... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://pdfium.googlesource.com/pdfium/+/b34d72b1045aafa420f6749b098e0ae6d9ce...
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
I don't think this is a good fix. You'll use a different clang version as the rest of the world, which sets you up for long-term pain. I'd recommend sending me a change to update.py that silently doesn't copy msdia*.dll if it doesn't exist.
Message was sent while issue was closed.
On 2016/04/18 19:27:26, Nico wrote: > I don't think this is a good fix. You'll use a different clang version as the > rest of the world, which sets you up for long-term pain. > > I'd recommend sending me a change to update.py that silently doesn't copy > msdia*.dll if it doesn't exist. I am having https://codereview.chromium.org/1900823002/ pending, which should be done very soon to solve this problem permanently. BTW, v8 is using an even older version of Clang.
Message was sent while issue was closed.
As far as I know, v8 has a pinned chromium and uses the clang of that pinned chromium -- and they update their pinned chromium very frequently. (They filed https://bugs.chromium.org/p/v8/issues/detail?id=4928 after Friday's clang roll, one day later)
Message was sent while issue was closed.
On 2016/04/18 19:31:23, Wei Li wrote: > On 2016/04/18 19:27:26, Nico wrote: > > I don't think this is a good fix. You'll use a different clang version as the > > rest of the world, which sets you up for long-term pain. > > > > I'd recommend sending me a change to update.py that silently doesn't copy > > msdia*.dll if it doesn't exist. > > I am having https://codereview.chromium.org/1900823002/ pending, which should be > done very soon to solve this problem permanently. That links to this cl? :-) > > BTW, v8 is using an even older version of Clang. |
