Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2351743004:
Added a target for a host version of the dart executable. (Closed)
|
Created:
4 years, 3 months ago by P.Y.L. Modified:
4 years, 3 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org Base URL:
https://chromium.googlesource.com/external/github.com/dart-lang/sdk/@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdded a target for a host version of the dart executable.
BUG=
R=zra@google.com
Committed: https://github.com/dart-lang/sdk/commit/94c96ac42c52c41e2a0e8c6e2ef1110b69faf9ac
Patch Set 1 #Patch Set 2 : Added a target for a host version of the dart executable. #
Total comments: 2
Patch Set 3 : Fixed third_party paths. #
Total comments: 2
Patch Set 4 : rebased #Patch Set 5 : removed 3p dir arg #
Total comments: 6
Patch Set 6 : rebased and flipped new arg #Patch Set 7 : removed extraneous dep #Patch Set 8 : of course I had to leave a spurious character #Messages
Total messages: 18 (3 generated)
Description was changed from ========== Added a target for a host version of the dart executable. BUG= ========== to ========== Added a target for a host version of the dart executable. BUG= ==========
pylaligand@google.com changed reviewers: + jamesr@chromium.org, zra@google.com
Sorry, had to create a new issue as I could not find a way to upload to the existing one (which was created on a different computer).
https://codereview.chromium.org/2351743004/diff/20001/runtime/bin/BUILD.gn File runtime/bin/BUILD.gn (right): https://codereview.chromium.org/2351743004/diff/20001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:347: rebase_path("//third_party/boringssl", "."), This will break the outside-of-Fuchsia Flutter engine build. Flutter DEPSes boringssl into //dart/third_party/boringssl. We can either change where Flutter puts boringssl, or keep and set the dart_boringssl_path build_arg in the Fuchsia host build.
https://codereview.chromium.org/2351743004/diff/20001/runtime/bin/BUILD.gn File runtime/bin/BUILD.gn (right): https://codereview.chromium.org/2351743004/diff/20001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:347: rebase_path("//third_party/boringssl", "."), On 2016/09/19 20:51:14, zra wrote: > This will break the outside-of-Fuchsia Flutter engine build. Flutter DEPSes > boringssl into //dart/third_party/boringssl. We can either change where Flutter > puts boringssl, or keep and set the dart_boringssl_path build_arg in the Fuchsia > host build. Ah, that's what I was missing. I thought the other kind of build had dart as the root, meaning //third_party/... would still have worked. Will update.
On 2016/09/19 20:58:34, P.Y.L. wrote: > https://codereview.chromium.org/2351743004/diff/20001/runtime/bin/BUILD.gn > File runtime/bin/BUILD.gn (right): > > https://codereview.chromium.org/2351743004/diff/20001/runtime/bin/BUILD.gn#ne... > runtime/bin/BUILD.gn:347: rebase_path("//third_party/boringssl", "."), > On 2016/09/19 20:51:14, zra wrote: > > This will break the outside-of-Fuchsia Flutter engine build. Flutter DEPSes > > boringssl into //dart/third_party/boringssl. We can either change where > Flutter > > puts boringssl, or keep and set the dart_boringssl_path build_arg in the > Fuchsia > > host build. > > Ah, that's what I was missing. I thought the other kind of build had dart as the > root, meaning //third_party/... would still have worked. Will update. Done.
https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn File runtime/bin/BUILD.gn (right): https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:271: dart_third_party_base + "/zlib", As it turns out, in the Flutter engine build, zlib comes from //third_party/zlib, and boringssl comes from //dart/third_party/boringssl. Sorry that this is a bit of a mess =(
https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn File runtime/bin/BUILD.gn (right): https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:271: dart_third_party_base + "/zlib", On 2016/09/19 21:14:27, zra wrote: > As it turns out, in the Flutter engine build, zlib comes from > //third_party/zlib, and boringssl comes from //dart/third_party/boringssl. Sorry > that this is a bit of a mess =( LOL. How about I leave zlib alone for now and we all gather to come up with a ling-term plan for this?
On 2016/09/19 21:16:47, P.Y.L. wrote: > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn > File runtime/bin/BUILD.gn (right): > > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn#ne... > runtime/bin/BUILD.gn:271: dart_third_party_base + "/zlib", > On 2016/09/19 21:14:27, zra wrote: > > As it turns out, in the Flutter engine build, zlib comes from > > //third_party/zlib, and boringssl comes from //dart/third_party/boringssl. > Sorry > > that this is a bit of a mess =( > > LOL. How about I leave zlib alone for now and we all gather to come up with a > ling-term plan for this? It should be easy enough to move boringssl in Flutter engine. I'll just go ahead try to do it now.
On 2016/09/19 21:20:32, zra wrote: > On 2016/09/19 21:16:47, P.Y.L. wrote: > > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn > > File runtime/bin/BUILD.gn (right): > > > > > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn#ne... > > runtime/bin/BUILD.gn:271: dart_third_party_base + "/zlib", > > On 2016/09/19 21:14:27, zra wrote: > > > As it turns out, in the Flutter engine build, zlib comes from > > > //third_party/zlib, and boringssl comes from //dart/third_party/boringssl. > > Sorry > > > that this is a bit of a mess =( > > > > LOL. How about I leave zlib alone for now and we all gather to come up with a > > ling-term plan for this? > > It should be easy enough to move boringssl in Flutter engine. I'll just go ahead > try to do it now. This is requiring a little yak shaving. Probably why no one's done it yet. Hopefully be able to get everything sorted out by tomorrow.
On 2016/09/19 22:28:44, zra wrote: > On 2016/09/19 21:20:32, zra wrote: > > On 2016/09/19 21:16:47, P.Y.L. wrote: > > > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn > > > File runtime/bin/BUILD.gn (right): > > > > > > > > > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn#ne... > > > runtime/bin/BUILD.gn:271: dart_third_party_base + "/zlib", > > > On 2016/09/19 21:14:27, zra wrote: > > > > As it turns out, in the Flutter engine build, zlib comes from > > > > //third_party/zlib, and boringssl comes from //dart/third_party/boringssl. > > > Sorry > > > > that this is a bit of a mess =( > > > > > > LOL. How about I leave zlib alone for now and we all gather to come up with > a > > > ling-term plan for this? > > > > It should be easy enough to move boringssl in Flutter engine. I'll just go > ahead > > try to do it now. > > This is requiring a little yak shaving. Probably why no one's done it yet. > Hopefully be able to get everything sorted out by tomorrow. Okay. This should be sorted, now. Now it is safe to assume that both zlib and boringssl are under //third_party.
On 2016/09/20 16:14:44, zra wrote: > On 2016/09/19 22:28:44, zra wrote: > > On 2016/09/19 21:20:32, zra wrote: > > > On 2016/09/19 21:16:47, P.Y.L. wrote: > > > > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn > > > > File runtime/bin/BUILD.gn (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn#ne... > > > > runtime/bin/BUILD.gn:271: dart_third_party_base + "/zlib", > > > > On 2016/09/19 21:14:27, zra wrote: > > > > > As it turns out, in the Flutter engine build, zlib comes from > > > > > //third_party/zlib, and boringssl comes from > //dart/third_party/boringssl. > > > > Sorry > > > > > that this is a bit of a mess =( > > > > > > > > LOL. How about I leave zlib alone for now and we all gather to come up > with > > a > > > > ling-term plan for this? > > > > > > It should be easy enough to move boringssl in Flutter engine. I'll just go > > ahead > > > try to do it now. > > > > This is requiring a little yak shaving. Probably why no one's done it yet. > > Hopefully be able to get everything sorted out by tomorrow. > > Okay. This should be sorted, now. Now it is safe to assume that both zlib and > boringssl are under //third_party. PTAL. Once this change goes in the Flutter folks will likely need to set the value of dart_third_party_base in their build.
On 2016/09/21 17:39:27, P.Y.L. wrote: > On 2016/09/20 16:14:44, zra wrote: > > On 2016/09/19 22:28:44, zra wrote: > > > On 2016/09/19 21:20:32, zra wrote: > > > > On 2016/09/19 21:16:47, P.Y.L. wrote: > > > > > > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn > > > > > File runtime/bin/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn#ne... > > > > > runtime/bin/BUILD.gn:271: dart_third_party_base + "/zlib", > > > > > On 2016/09/19 21:14:27, zra wrote: > > > > > > As it turns out, in the Flutter engine build, zlib comes from > > > > > > //third_party/zlib, and boringssl comes from > > //dart/third_party/boringssl. > > > > > Sorry > > > > > > that this is a bit of a mess =( > > > > > > > > > > LOL. How about I leave zlib alone for now and we all gather to come up > > with > > > a > > > > > ling-term plan for this? > > > > > > > > It should be easy enough to move boringssl in Flutter engine. I'll just go > > > ahead > > > > try to do it now. > > > > > > This is requiring a little yak shaving. Probably why no one's done it yet. > > > Hopefully be able to get everything sorted out by tomorrow. > > > > Okay. This should be sorted, now. Now it is safe to assume that both zlib and > > boringssl are under //third_party. > > PTAL. Once this change goes in the Flutter folks will likely need to set the > value of dart_third_party_base in their build. Both Flutter and Dart have all their third_party stuff under //third_party, now, so we can just write that directly instead of using an argument.
On 2016/09/21 17:43:25, zra wrote: > On 2016/09/21 17:39:27, P.Y.L. wrote: > > On 2016/09/20 16:14:44, zra wrote: > > > On 2016/09/19 22:28:44, zra wrote: > > > > On 2016/09/19 21:20:32, zra wrote: > > > > > On 2016/09/19 21:16:47, P.Y.L. wrote: > > > > > > > > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn > > > > > > File runtime/bin/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2351743004/diff/40001/runtime/bin/BUILD.gn#ne... > > > > > > runtime/bin/BUILD.gn:271: dart_third_party_base + "/zlib", > > > > > > On 2016/09/19 21:14:27, zra wrote: > > > > > > > As it turns out, in the Flutter engine build, zlib comes from > > > > > > > //third_party/zlib, and boringssl comes from > > > //dart/third_party/boringssl. > > > > > > Sorry > > > > > > > that this is a bit of a mess =( > > > > > > > > > > > > LOL. How about I leave zlib alone for now and we all gather to come up > > > with > > > > a > > > > > > ling-term plan for this? > > > > > > > > > > It should be easy enough to move boringssl in Flutter engine. I'll just > go > > > > ahead > > > > > try to do it now. > > > > > > > > This is requiring a little yak shaving. Probably why no one's done it yet. > > > > Hopefully be able to get everything sorted out by tomorrow. > > > > > > Okay. This should be sorted, now. Now it is safe to assume that both zlib > and > > > boringssl are under //third_party. > > > > PTAL. Once this change goes in the Flutter folks will likely need to set the > > value of dart_third_party_base in their build. > > Both Flutter and Dart have all their third_party stuff under //third_party, now, > so we can just write that directly instead of using an argument. Even better. Done!
lgtm with small fixes. https://codereview.chromium.org/2351743004/diff/80001/runtime/bin/BUILD.gn File runtime/bin/BUILD.gn (right): https://codereview.chromium.org/2351743004/diff/80001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:347: rebase_path("//third_party/boringssl", "."), rebase_path no longer needed https://codereview.chromium.org/2351743004/diff/80001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:371: if (is_linux && !dart_no_fallback_root_certificates) { maybe flip the sense to dart_use_fallback... to avoid "not no fallback" https://codereview.chromium.org/2351743004/diff/80001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:474: "../../pkg:pkg", # Pull this out to top-level for a real SDK build. I think this doesn't exist anymore. Do you need to rebase?
https://codereview.chromium.org/2351743004/diff/80001/runtime/bin/BUILD.gn File runtime/bin/BUILD.gn (right): https://codereview.chromium.org/2351743004/diff/80001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:347: rebase_path("//third_party/boringssl", "."), On 2016/09/21 17:55:31, zra wrote: > rebase_path no longer needed Done. https://codereview.chromium.org/2351743004/diff/80001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:371: if (is_linux && !dart_no_fallback_root_certificates) { On 2016/09/21 17:55:31, zra wrote: > maybe flip the sense to dart_use_fallback... to avoid "not no fallback" Done. https://codereview.chromium.org/2351743004/diff/80001/runtime/bin/BUILD.gn#ne... runtime/bin/BUILD.gn:474: "../../pkg:pkg", # Pull this out to top-level for a real SDK build. On 2016/09/21 17:55:31, zra wrote: > I think this doesn't exist anymore. Do you need to rebase? Aaah, the rebase had failed quite silently. Should be good now.
Description was changed from ========== Added a target for a host version of the dart executable. BUG= ========== to ========== Added a target for a host version of the dart executable. BUG= R=zra@google.com Committed: https://github.com/dart-lang/sdk/commit/94c96ac42c52c41e2a0e8c6e2ef1110b69faf9ac ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as 94c96ac42c52c41e2a0e8c6e2ef1110b69faf9ac (presubmit successful). |
