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

Feature/multitarget #89

Closed
wants to merge 6 commits into from
Closed

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Nov 3, 2022

Fixes #80 (?)

Introduction

Attempts to bring back support for Android, iOS and MacCatalyst, removed in 954f936.

How to compile?

To compile code in this branch, one has to call:

dotnet workload install android
dotnet workload install ios
dotnet workload install maccatalyst

Now ideally restart Visual Studio, if that is your IDE.

Testing

I have run unit tests and they pass for me. I'm not sure how to test further. Any ideas?

Any feedback is much appreciated.

@MartyIX
Copy link
Contributor Author

MartyIX commented Nov 3, 2022

@ektrah Could you please allow CI runs for this PR for me to know if I got it right or wrong?

edit: Tested here: https://github.com/MartyIX/libsodium-core/actions/runs/3383999101/jobs/5620456128

@MartyIX MartyIX force-pushed the feature/multitarget branch 3 times, most recently from c7391ec to 737576f Compare November 3, 2022 07:54
@MartyIX
Copy link
Contributor Author

MartyIX commented Nov 7, 2022

@ektrah Does the PR make sense to you? Or do you think the approach is wrong?

@ektrah
Copy link
Owner

ektrah commented Nov 7, 2022

Thanks a lot for tackling this!

The PR looks quite good so far. A few comments:

  • With the PR, the GitHub Action now builds a NuGet package that includes .dll's for net6.0, net6.0-android31.0, net6.0-ios16.0, and net6.0-maccatalyst15.4 in addition to netstandard2.1 and netstandard2.0. It would be great if the GitHub Action could be enhanced to show that it actually works (e.g., by not just building the .dll's for those platforms but also running the unit tests on them or by compiling a test project that references the built NuGet package or similar).
  • There is some older documentation in Xamarin.md. Is that still relevant? Could it be updated to the .NET 6 way of doing things?

@MartyIX
Copy link
Contributor Author

MartyIX commented Nov 10, 2022

  • It would be great if the GitHub Action could be enhanced to show that it actually works (e.g., by not just building the .dll's for those platforms but also running the unit tests on them or [..].

I've been slowly researching this but it does not seem to be as simple as adding dotnet test -f net6.0-maccatalyst -c Debug.

  • There is some older documentation in Xamarin.md. Is that still relevant? Could it be updated to the .NET 6 way of doing things?

Not an expert on this. It would be great if the original author could give a hand :)

@ektrah
Copy link
Owner

ektrah commented Nov 27, 2022

/cc @enclave-alistair (who seems to be working on iOS support for another libsodium binding)

@MartyIX
Copy link
Contributor Author

MartyIX commented Nov 27, 2022

So I have been struggling with testing this:

@MartyIX
Copy link
Contributor Author

MartyIX commented Dec 11, 2022

So I have found out that .NET 7 console application on macOS works OK:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net7.0</TargetFramework>
    <RootNamespace>libsodium_core_console</RootNamespace>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
      <PackageReference Include="Sodium.Core" Version="1.3.1" />
  </ItemGroup>
</Project>
using Sodium;

internal class Program
{
    private static void Main(string[] args)
    {
        Console.WriteLine(SecretAeadAes.IsAvailable);
    }
}

The program above does not crash.

However, a similar .NET 7 MacCatalyst application crashes because it cannot find the libsodium DLL:

namespace libsodium_core_maccatalyst;

using Sodium;

[Register ("AppDelegate")]
public class AppDelegate : UIApplicationDelegate {
	public override UIWindow? Window {
		get;
		set;
	}

	public override bool FinishedLaunching (UIApplication application, NSDictionary launchOptions)
	{
		// create a new window instance based on the screen size
		Window = new UIWindow (UIScreen.MainScreen.Bounds);

		// create a UIViewController with a single UILabel
		var vc = new UIViewController ();
		vc.View!.AddSubview (new UILabel (Window!.Frame) {
			BackgroundColor = UIColor.SystemBackground,
			TextAlignment = UITextAlignment.Center,
			Text = SecretAeadAes.IsAvailable.ToString(), // MODIFIED LINE.
			AutoresizingMask = UIViewAutoresizing.All,
		});
		Window.RootViewController = vc;

		// make the window visible
		Window.MakeKeyAndVisible ();

		return true;
	}
}

So the problem is clear. The solution is not. :|

edit: I have asked a question here: dotnet/runtime#79502

@ektrah
Copy link
Owner

ektrah commented Dec 11, 2022

@MartyIX, thanks a lot for looking into this! Your findings show that there really should be some tests 🙂 I'm afraid I can't provide much help here, as I've never done any .NET ios/android/maccatalyst development.

@MartyIX
Copy link
Contributor Author

MartyIX commented Dec 12, 2022

Asked here jedisct1/libsodium#1235 as the issue is in libsodium and not in this library. At least as far as I understands it based on dotnet/runtime#79502 (reply in thread).

@MartyIX MartyIX closed this Jan 10, 2023
@MartyIX
Copy link
Contributor Author

MartyIX commented Jul 31, 2023

@ektrah So now we wait until libsodium releases (jedisct1/libsodium#1238 (comment)) so that this package can be modified to provide support for mac catalyst. But given jedisct1/libsodium#1136 there is no estimate I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS and Android support
2 participants