Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove openssl dependency from android #49282

Merged
merged 29 commits into from
Mar 23, 2021

Conversation

steveisok
Copy link
Member

@steveisok steveisok commented Mar 7, 2021

Stops including openssl in the ci / official builds for android by not providing ANDROID_OPENSSL_AAR.

@ghost
Copy link

ghost commented Mar 7, 2021

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

We've reached a point where we can run a good number of tests w/o depending on openssl. We should flip the switch permanently.

Author: steveisok
Assignees: -
Labels:

area-Infrastructure-mono

Milestone: -

@steveisok
Copy link
Member Author

It's in draft status for now so that I can go through all the existing test failures and skip them.

@elinor-fung
Copy link
Member

FYI, I think you're going to hit crashes in a bunch of the System.Net.* tests (ones that use Configuration.Certificates.cs) right now. They use certs that rely on RC2, which throws PNSE on Android (and the tests fail fast / Trace.Fail if there's an error reading the certs). We'll need to consume the new test data from dotnet/runtime-assets#120.

@jkoritzinsky
Copy link
Member

I think we should rename the native Android crypto interop library to not have OpenSsl in the name now that we're removing support for the OpenSSL interop lib on Android.

@elinor-fung
Copy link
Member

rename the native Android crypto interop library to not have OpenSsl in the name

Agreed. There's an item in #45741 calling that out. I think that's fine to have in a separate change though, since it will require some of refactoring in how we define/include the library names, which is not necessarily related/required for simply dropping the OpenSSL dependency and getting CI with tests going.

@elinor-fung
Copy link
Member

There seems to still be something fishy with failure reporting?
For example, in the run before the latest update, the Android leg succeeded (fully succeeded, not partially succeeded), but the log for Android.Device_Emulator.PInvoke.Test shows it crashed:
https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-49282-merge-34b7bb71b72d4cebb5/Android.Device_Emulator.PInvoke.Test/console.1cc3192b.log?sv=2019-07-07&se=2021-04-11T18%3A47%3A51Z&sr=c&sp=rl&sig=DZSdqaxBqio2drs7nwY0XtigCUvVrbXrLaX%2BsLyfz4U%3D

@elinor-fung
Copy link
Member

A bunch of others (including System.Security.Cryptography.*) crashed too.

Maybe the issue with reporting is dotnet/xharness#446 (comment)

@steveisok
Copy link
Member Author

@elinor-fung I did see that too.

I opened this w/ core-eng. The title is now misleading, but it does refer to what you're talking about.

@steveisok steveisok merged commit bc5b227 into dotnet:main Mar 23, 2021
@steveisok steveisok deleted the disable-openssl-ci branch March 23, 2021 04:17
@steveisok steveisok restored the disable-openssl-ci branch March 23, 2021 04:18
@steveisok
Copy link
Member Author

/backport to release/6.0-preview3

@github-actions
Copy link
Contributor

Started backporting to release/6.0-preview3: https:/dotnet/runtime/actions/runs/680235515

@github-actions
Copy link
Contributor

@steveisok backporting to release/6.0-preview3 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Disable openssl dep for all of android. See what fails on CI first
Using index info to reconstruct a base tree...
M	eng/pipelines/runtime-staging.yml
M	eng/pipelines/runtime.yml
M	src/libraries/tests.proj
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/tests.proj
Auto-merging eng/pipelines/runtime.yml
Auto-merging eng/pipelines/runtime-staging.yml
CONFLICT (content): Merge conflict in eng/pipelines/runtime-staging.yml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Disable openssl dep for all of android. See what fails on CI first
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

steveisok added a commit to steveisok/runtime that referenced this pull request Mar 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 22, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants