Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(894)

Issue 1809153002: Removing iOS from testing in BoringSSL (Closed)

Created:
4 years, 9 months ago by svaldez
Modified:
4 years, 6 months ago
Reviewers:
davidben
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix BoringSSL build on iOS In order to get BoringSSL to build on iOS we need to make a couple of initial changes: * We disable unittests due to the current inability to run new processes on iOS. * We disable the use of custom assembly since we are unable to build the assembly for the iOS architecture and would otherwise be unable to test it. BUG=591545 Committed: https://crrev.com/2e34051065f7e6bd139780273148d7d0c643485f Cr-Commit-Position: refs/heads/master@{#381812}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -18 lines) Patch
M third_party/boringssl/BUILD.gn View 2 chunks +6 lines, -2 lines 0 comments Download
M third_party/boringssl/boringssl.gyp View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/boringssl/boringssl_tests.gyp View 1 chunk +18 lines, -14 lines 0 comments Download

Messages

Total messages: 17 (7 generated)
svaldez
4 years, 9 months ago (2016-03-17 17:16:51 UTC) #2
davidben
lgtm. Probably want to tweak the description to mention why boringssl_unittests can't be built and ...
4 years, 9 months ago (2016-03-17 17:31:45 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809153002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809153002/1
4 years, 9 months ago (2016-03-17 17:35:25 UTC) #6
commit-bot: I haz the power
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/builds/118878)
4 years, 9 months ago (2016-03-17 19:12:51 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809153002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809153002/1
4 years, 9 months ago (2016-03-17 21:30:17 UTC) #10
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 9 months ago (2016-03-17 22:38:13 UTC) #12
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/2e34051065f7e6bd139780273148d7d0c643485f Cr-Commit-Position: refs/heads/master@{#381812}
4 years, 9 months ago (2016-03-17 22:39:32 UTC) #14
efimovmichael
On 2016/03/17 17:31:45, davidben (OOO until 5-31) wrote: > lgtm. > > Probably want to ...
4 years, 6 months ago (2016-06-06 13:34:31 UTC) #15
chromium-reviews
Do you know if there's some better way of getting the iOS configuration arch? We ...
4 years, 6 months ago (2016-06-06 13:53:31 UTC) #16
davidben
4 years, 6 months ago (2016-06-06 14:48:06 UTC) #17
Message was sent while issue was closed.
Huh, I thought we took iOS out of the assembly picture completely. That some iOS
mentions are still there were an oversight. We definitely need to fix that.

On 2016/06/06 13:53:31, chromium-reviews wrote:
> Do you know if there's some better way of getting the iOS configuration
> arch? We can probably just move the NO_ASM check outside of the target_arch
> for now since we aren't shipping iOS assembly yet, but it would be nice to
> have the correct architecture sources for when we actually enable iOS
> assembly.

As I understand, we can't. The iOS build story is such that XCode handles the
architecture switch, not gyp. Which means we can't do anything conditioned on
architecture in gyp at all. We'd have to have a completely different iOS story
(maybe wrap the assembly in ifdefs or something).

> -Steven
> 
> On Mon, Jun 6, 2016 at 9:34 AM <mailto:efimovmichael@yandex-team.ru> wrote:
> 
> > On 2016/03/17 17:31:45, davidben (OOO until 5-31) wrote:
> > > lgtm.
> > >
> > > Probably want to tweak the description to mention why boringssl_unittests
> > can't
> > > be built and the assembly issue. (I think it's because target_arch is
> > weird on
> > > iOS? Something about arch-specific bits being handled in XCode.) May as
> > well
> > > staple it to the bug too.
> >
> > Hi! There is a problem, which was introduced in
> > https://codereview.chromium.org/1126663002 and was not fixed here.
> > "OS==ios" conditions are added inside "target_arch=..." conditions, but
> > all iOS
> > configurations have the same "target_arch" which is "ia32".
> > This leads to some issues:
> > 1. "OS==ios" conditions inside "target_arch=x64" conditions are not used
> > at all.
> > 2. All iOS configurations(including x64 and both arm-s) actually include
> > "boringssl_mac_x86_sources" sources.
> > 3. Starting from this commit, all iOS configurations also have
> > "OPENSSL_NO_ASM"
> > key, that means they include all "generic" implementations. This leads to
> > duplicate symbols on "i386" arch.
> >
> > https://codereview.chromium.org/1809153002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698