Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
771593002:
Add rule to build nacl_helper_nonsfi for ARM (Closed)
DescriptionAdd rule to build nacl_helper_nonsfi for ARM
- Added gyp rule to build nacl_helper_nonsfi for ARM
- Fixed the target flag to use armv7-unknown-nacl-gnueabihf
BUG= https://code.google.com/p/chromium/issues/detail?id=433201
BUG= https://code.google.com/p/chromium/issues/detail?id=372049
TEST=try
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=14230
Patch Set 1 : #
Total comments: 2
Patch Set 2 : address comment #
Total comments: 4
Patch Set 3 : address comment #
Messages
Total messages: 23 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
mazda@chromium.org changed reviewers: + hidehiko@chromium.org, mseaborn@chromium.org
PTAL All NaCl browser tests passed with Chrome side change and stack alignment fix applied. https://codereview.chromium.org/794573002/ https://codereview.chromium.org/722423003/
https://codereview.chromium.org/771593002/diff/60001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/771593002/diff/60001/build/untrusted.gypi#new... build/untrusted.gypi:886: '<(DEPTH)/native_client/build/build_nexe.py', common_inputs ?
https://codereview.chromium.org/771593002/diff/60001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/771593002/diff/60001/build/untrusted.gypi#new... build/untrusted.gypi:886: '<(DEPTH)/native_client/build/build_nexe.py', On 2014/12/11 11:39:58, hidehiko wrote: > common_inputs ? Done.
lgtm
LGTM https://codereview.chromium.org/771593002/diff/80001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/771593002/diff/80001/build/untrusted.gypi#new... build/untrusted.gypi:909: # "-nodefaultlibs -Wl,--starg-group, ... -Wl,--end-group" Copied typo: "--start-group". (You might as well fix the existing instance of this.) https://codereview.chromium.org/771593002/diff/80001/src/nonsfi/loader/loader... File src/nonsfi/loader/loader.gyp (right): https://codereview.chromium.org/771593002/diff/80001/src/nonsfi/loader/loader... src/nonsfi/loader/loader.gyp:48: 'extra_deps_newlib_arm_nonsfi': [ Will similar changes be needed on the Chromium side when rolling this into Chromium? If so, could you commit those changes before committing this?
Thank you for the review. https://codereview.chromium.org/771593002/diff/80001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/771593002/diff/80001/build/untrusted.gypi#new... build/untrusted.gypi:909: # "-nodefaultlibs -Wl,--starg-group, ... -Wl,--end-group" On 2014/12/15 03:16:32, Mark Seaborn wrote: > Copied typo: "--start-group". (You might as well fix the existing instance of > this.) Done. https://codereview.chromium.org/771593002/diff/80001/src/nonsfi/loader/loader... File src/nonsfi/loader/loader.gyp (right): https://codereview.chromium.org/771593002/diff/80001/src/nonsfi/loader/loader... src/nonsfi/loader/loader.gyp:48: 'extra_deps_newlib_arm_nonsfi': [ On 2014/12/15 03:16:32, Mark Seaborn wrote: > Will similar changes be needed on the Chromium side when rolling this into > Chromium? If so, could you commit those changes before committing this? Yes, similar changes like the following are needed on the Chromium. https://codereview.chromium.org/794573002/diff/20001/components/nacl_nonsfi.gyp Let me clarify the commit order of the related CLs. 1. The similar change on Chromium side (https://codereview.chromium.org/794573002/diff/20001/components/nacl_nonsfi.gyp) 2. This CL 3. (After NaCl is rolled), Chrome side change (https://codereview.chromium.org/794573002/) except the change hunk in 1. Is my understanding correct?
On 17 December 2014 at 09:11, <mazda@chromium.org> wrote: > https://codereview.chromium.org/771593002/diff/80001/src/ > nonsfi/loader/loader.gyp#newcode48 > src/nonsfi/loader/loader.gyp:48: 'extra_deps_newlib_arm_nonsfi': [ > On 2014/12/15 03:16:32, Mark Seaborn wrote: > >> Will similar changes be needed on the Chromium side when rolling this into > > Chromium? If so, could you commit those changes before committing this? > > > Yes, similar changes like the following are needed on the Chromium. > https://codereview.chromium.org/794573002/diff/20001/ > components/nacl_nonsfi.gyp > > Let me clarify the commit order of the related CLs. > > 1. The similar change on Chromium side > (https://codereview.chromium.org/794573002/diff/20001/ > components/nacl_nonsfi.gyp) > 2. This CL > 3. (After NaCl is rolled), Chrome side change > (https://codereview.chromium.org/794573002/) except the change hunk in > 1. > > Is my understanding correct? So you're saying that you need to split https://codereview.chromium.org/794573002/ into two pieces? If that's the case, I guess you'd better do that so you can get the first piece committed before step 2 above. :-) However, your (ARM-only) trybot runs for https://codereview.chromium.org/794573002/ seem to pass. Does it pass on the other bots too? If so, I guess you could commit all of it for step 1. However, looking at the trybot run, I see you have native_client patches applied in there via an apply_nacl_patch.py script. Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
> So you're saying that you need to split > https://codereview.chromium.org/794573002/ into two pieces? If that's the > case, I guess you'd better do that so you can get the first piece committed > before step 2 above. :-) OK, I split the CL into two. https://codereview.chromium.org/818703002/ https://codereview.chromium.org/794573002/ Even without splitting the CL, build bots would not break because browser tests using nacl_helper_nonsfi on ARM are not used yet. However when building nacl_helper_nonsfi locally after this CL is rolled in, that would cause dependency issue. > However, your (ARM-only) trybot runs for > https://codereview.chromium.org/794573002/ seem to pass. Does it pass on > the other bots too? If so, I guess you could commit all of it for step 1. > However, looking at the trybot run, I see you have native_client patches > applied in there via an apply_nacl_patch.py script. Right. trybot runs for https://codereview.chromium.org/794573002/ passes by applying this patch.
The CQ bit was checked by mazda@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/771593002/100001
The CQ bit was unchecked by sheyang@chromium.org
On 2014/12/19 16:30:13, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/771593002/100001 Somehow CQ crashed when processing this CL. I'm unchecking this CL before restarting CQ, and will investigate.
The CQ bit was checked by sheyang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/771593002/100001
The CQ bit was unchecked by sheyang@chromium.org
The CQ bit was checked by sheyang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/771593002/100001
Nacl CQ seems to work now. Sorry for the inconvenience. Best regards, Sheng On Fri, Dec 19, 2014 at 9:22 AM, <commit-bot@chromium.org> wrote: > > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/771593002/100001 > > > https://codereview.chromium.org/771593002/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as http://src.chromium.org/viewvc/native_client?view=rev&revision=14230 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
