Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2389133002:
Add support for component builds on Windows Swarming. (Closed)
|
Created:
4 years, 2 months ago by ehmaldonado_webrtc Modified:
4 years, 2 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd support for component builds on Windows Swarming.
Some runtime dependencies on MSVC were missing and had to be added to rtc_base_approved.
BUG=chromium:497757
NOTRY=True
Committed: https://crrev.com/66d4ce93244d17eb52a409dde036fe9114937651
Cr-Commit-Position: refs/heads/master@{#14562}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Filed a bug and added TODO. #
Total comments: 1
Messages
Total messages: 23 (12 generated)
Description was changed from ========== stuff BUG= ========== to ========== stuff BUG= ==========
ehmaldonado@webrtc.org changed reviewers: + kjellander@webrtc.org
Super ugly CL not ready for commit yet. What would you think about something like this. I'm putting it in the templates. I had to gut audio_coding/audio_coding.gni because it seems like it includes the visual studio version gni twice and complains about clobbering. I hope to do a workaround, but maybe you have a better idea.
I think it makes more sense to put this into webrtc/base:rtc_base_approved or similar library that (I think) all tests depend on (we'll figure out when we run the a Win Debug swarming trybot). The duplication in the template is something I'd really prefer to avoid. Since Win swarming is not a hurry (Android is priority) we should also reach out to brucedawson@ who wrote https://codereview.chromium.org/1783973002 to ask for advice on how best refactor it to make it useful for client projects.
Patchset #4 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== stuff BUG= ========== to ========== Add support for component builds on Windows Swarming. Some run time dependencies on MSVC were missing and had to be added to rtc_base_approved. BUG=chromium:497757 ==========
On 2016/10/05 07:44:44, kjellander_webrtc wrote: > I think it makes more sense to put this into webrtc/base:rtc_base_approved or > similar library that (I think) all tests depend on (we'll figure out when we run > the a Win Debug swarming trybot). The duplication in the template is something > I'd really prefer to avoid. > > Since Win swarming is not a hurry (Android is priority) we should also reach out > to brucedawson@ who wrote https://codereview.chromium.org/1783973002 to ask for > advice on how best refactor it to make it useful for client projects. PTAL.
https://codereview.webrtc.org/2389133002/diff/80001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2389133002/diff/80001/webrtc/base/BUILD.gn#newc... webrtc/base/BUILD.gn:202: # Copy the VS runtime DLLs into the isolate so that they don't have to be Can you file a Type=Feature bug at crbug.com where you suggest the stuff added in https://codereview.chromium.org/1783973002 is moved out from src/build into something in src/build so it's easier to use. It seems this is not really Chromium-specific Make sure to CC dpranke@ and brucedawson@ at it. Without that, this is surely going to cause problems in the future when something changes regarding which files are needed. Then add a TODO here about that we'd like to use that instead, referencing the crbug.
On 2016/10/06 09:25:06, kjellander_webrtc wrote: > https://codereview.webrtc.org/2389133002/diff/80001/webrtc/base/BUILD.gn > File webrtc/base/BUILD.gn (right): > > https://codereview.webrtc.org/2389133002/diff/80001/webrtc/base/BUILD.gn#newc... > webrtc/base/BUILD.gn:202: # Copy the VS runtime DLLs into the isolate so that > they don't have to be > Can you file a Type=Feature bug at http://crbug.com where you suggest the stuff added > in https://codereview.chromium.org/1783973002 is moved out from src/build into > something in src/build so it's easier to use. It seems this is not really > Chromium-specific > Make sure to CC dpranke@ and brucedawson@ at it. > Without that, this is surely going to cause problems in the future when > something changes regarding which files are needed. > > Then add a TODO here about that we'd like to use that instead, referencing the > crbug. PTAL
Description was changed from ========== Add support for component builds on Windows Swarming. Some run time dependencies on MSVC were missing and had to be added to rtc_base_approved. BUG=chromium:497757 ========== to ========== Add support for component builds on Windows Swarming. Some run time dependencies on MSVC were missing and had to be added to rtc_base_approved. BUG=chromium:497757 NOTRY=True ==========
Description was changed from ========== Add support for component builds on Windows Swarming. Some run time dependencies on MSVC were missing and had to be added to rtc_base_approved. BUG=chromium:497757 NOTRY=True ========== to ========== Add support for component builds on Windows Swarming. Some runtime dependencies on MSVC were missing and had to be added to rtc_base_approved. BUG=chromium:497757 NOTRY=True ==========
Nice! lgtm I added NOTRY=True for you.
The CQ bit was checked by ehmaldonado@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add support for component builds on Windows Swarming. Some runtime dependencies on MSVC were missing and had to be added to rtc_base_approved. BUG=chromium:497757 NOTRY=True ========== to ========== Add support for component builds on Windows Swarming. Some runtime dependencies on MSVC were missing and had to be added to rtc_base_approved. BUG=chromium:497757 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #2 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add support for component builds on Windows Swarming. Some runtime dependencies on MSVC were missing and had to be added to rtc_base_approved. BUG=chromium:497757 NOTRY=True ========== to ========== Add support for component builds on Windows Swarming. Some runtime dependencies on MSVC were missing and had to be added to rtc_base_approved. BUG=chromium:497757 NOTRY=True Committed: https://crrev.com/66d4ce93244d17eb52a409dde036fe9114937651 Cr-Commit-Position: refs/heads/master@{#14562} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/66d4ce93244d17eb52a409dde036fe9114937651 Cr-Commit-Position: refs/heads/master@{#14562}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:100001) has been created in https://codereview.webrtc.org/2400953002/ by ehmaldonado@webrtc.org. The reason for reverting is: Causes compile errors. https://build.chromium.org/p/client.webrtc/builders/Mac64%20Debug/builds/8646....
Message was sent while issue was closed.
https://codereview.webrtc.org/2389133002/diff/100001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2389133002/diff/100001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:201: if (is_component_build) { Add "&& is_win" to the condition, as you've probably found out now :) It's always a good idea to run a few more bots, at least one per platform. But I also didn't think about that. |
