Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2224783002:
clang update script: Verify that VERSION is correct. (Closed)
|
Created:
4 years, 4 months ago by Nico Modified:
4 years, 4 months ago Reviewers:
hans CC:
chromium-reviews, eugenis+clang_chromium.org, vmpstr+watch_chromium.org, yunlian, Reid Kleckner, glider+clang_chromium.org, Nico, ukai+watch_chromium.org, hans, dmikurube+clang_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionclang update script: Verify that VERSION is correct.
BUG=629966
Committed: https://crrev.com/1a12f0dc15e89907a833f7ad38e91bc89d4ef4e4
Cr-Commit-Position: refs/heads/master@{#410467}
Patch Set 1 #
Total comments: 2
Patch Set 2 : check on win #
Total comments: 5
Patch Set 3 : regex #Messages
Total messages: 22 (10 generated)
The CQ bit was checked by thakis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thakis@chromium.org changed reviewers: + hans@chromium.org
Should've done that half a year ago, would've saved me some time this time round :-(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2224783002/diff/1/tools/clang/scripts/update.py File tools/clang/scripts/update.py (right): https://codereview.chromium.org/2224783002/diff/1/tools/clang/scripts/update.... tools/clang/scripts/update.py:381: # FIXME: Parse `clang-cl /?` output for built clang's version and check Doesn't the code below work on Windows too? Regular clang.exe is still there.
done https://codereview.chromium.org/2224783002/diff/1/tools/clang/scripts/update.py File tools/clang/scripts/update.py (right): https://codereview.chromium.org/2224783002/diff/1/tools/clang/scripts/update.... tools/clang/scripts/update.py:381: # FIXME: Parse `clang-cl /?` output for built clang's version and check On 2016/08/08 16:11:23, hans wrote: > Doesn't the code below work on Windows too? Regular clang.exe is still there. That's a good point. I figured it'd be nicer to get it from the binary we're actually shipping, but going to clang.exe is probably better than doing nothing on Windows (though as soon as this runs on any one platform it'll do its purpose). Let me upload a new patch...
(please hit cq if you like this now)
https://codereview.chromium.org/2224783002/diff/20001/tools/clang/scripts/upd... File tools/clang/scripts/update.py (right): https://codereview.chromium.org/2224783002/diff/20001/tools/clang/scripts/upd... tools/clang/scripts/update.py:385: clang += '.exe' Yeah, I think checking clang is good enough. If we *really* wanted to, we could do "clang-cl.exe --driver-mode=gcc", which we could pass --version too and it will behave just like clang.exe. But that also seems like a lot of hoops to jump through. https://codereview.chromium.org/2224783002/diff/20001/tools/clang/scripts/upd... tools/clang/scripts/update.py:387: version_out = re.match(r'clang version (\S+)', version_out).group(1) Sorry, I should have mentioned this the first time: Perhaps \S might be too broad? Could we check for [0-9.]+ instead?
https://codereview.chromium.org/2224783002/diff/20001/tools/clang/scripts/upd... File tools/clang/scripts/update.py (right): https://codereview.chromium.org/2224783002/diff/20001/tools/clang/scripts/upd... tools/clang/scripts/update.py:387: version_out = re.match(r'clang version (\S+)', version_out).group(1) On 2016/08/08 18:54:00, hans wrote: > Sorry, I should have mentioned this the first time: Perhaps \S might be too > broad? Could we check for [0-9.]+ instead? Does it matter if we compare to VERSION later? If it always matches too much we'll find out then. But I can make it [0-9.] too if you think it' s important.
https://codereview.chromium.org/2224783002/diff/20001/tools/clang/scripts/upd... File tools/clang/scripts/update.py (right): https://codereview.chromium.org/2224783002/diff/20001/tools/clang/scripts/upd... tools/clang/scripts/update.py:387: version_out = re.match(r'clang version (\S+)', version_out).group(1) On 2016/08/08 19:00:31, Nico (away until Mon Aug 22) wrote: > On 2016/08/08 18:54:00, hans wrote: > > Sorry, I should have mentioned this the first time: Perhaps \S might be too > > broad? Could we check for [0-9.]+ instead? > > Does it matter if we compare to VERSION later? If it always matches too much > we'll find out then. But I can make it [0-9.] too if you think it' s important. I figured if for whatever reason clang starts printing "4.0.0-trunk" instead of just "4.0.0" or whatever, that would unnecessarily break this test. Sure we'd catch it, but it would be annoying.
The CQ bit was checked by thakis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2224783002/diff/20001/tools/clang/scripts/upd... File tools/clang/scripts/update.py (right): https://codereview.chromium.org/2224783002/diff/20001/tools/clang/scripts/upd... tools/clang/scripts/update.py:387: version_out = re.match(r'clang version (\S+)', version_out).group(1) On 2016/08/08 19:27:32, hans wrote: > On 2016/08/08 19:00:31, Nico (away until Mon Aug 22) wrote: > > On 2016/08/08 18:54:00, hans wrote: > > > Sorry, I should have mentioned this the first time: Perhaps \S might be too > > > broad? Could we check for [0-9.]+ instead? > > > > Does it matter if we compare to VERSION later? If it always matches too much > > we'll find out then. But I can make it [0-9.] too if you think it' s > important. > > I figured if for whatever reason clang starts printing "4.0.0-trunk" instead of > just "4.0.0" or whatever, that would unnecessarily break this test. Sure we'd > catch it, but it would be annoying. Hm, I think I'd prefer if this turned red in that case (maybe the headers are now in include/clang/4.0.0-trunk too) so we'd catch if this changes in surprising ways. But I also think it's less important than the time we're going to spend arguing about this, so done :-)
lgtm
The CQ bit was unchecked by thakis@chromium.org
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== clang update script: Verify that VERSION is correct. BUG=629966 ========== to ========== clang update script: Verify that VERSION is correct. BUG=629966 Committed: https://crrev.com/1a12f0dc15e89907a833f7ad38e91bc89d4ef4e4 Cr-Commit-Position: refs/heads/master@{#410467} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1a12f0dc15e89907a833f7ad38e91bc89d4ef4e4 Cr-Commit-Position: refs/heads/master@{#410467} |
