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

First task to create a nuget package and remove sources from mono/mono #288

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

thaystg
Copy link
Contributor

@thaystg thaystg commented Jan 31, 2020

Removing usage of AsyncCallback and AsyncResult, this is not available in netstandard.

Changing projects to use new csproj style.

…e in netstandard.

Changing projects to use new csproj style.
<PackageReference Include="Newtonsoft.Json">
<Version>10.0.3</Version>
</PackageReference>
<ProjectReference Include="..\Mono.Debugging\Mono.Debugging.csproj" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing marking projects as private.

AsyncResult result = (AsyncResult) asyncResult;
LaunchCallback cb = (LaunchCallback) result.AsyncDelegate;
return cb.EndInvoke (asyncResult);
asyncResult.Wait ();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.Wait() is not needed if we're doing .Result.

One other remark is that we probably don't want to force a .Result here. Maybe we need a public static Task<VirtualMachine> Launch()?

@@ -111,20 +111,19 @@ public static IAsyncResult BeginLaunch (ProcessStartInfo info, AsyncCallback cal
socket.Close ();
};

LaunchCallback c = new LaunchCallback (LaunchInternal);
return c.BeginInvoke (p, info, socket, callback, socket);
Task<VirtualMachine> t3 = Task<VirtualMachine>.Run (() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing a Task.Run here does not mirror previous functionality. Maybe simplify it to:

return LaunchInternal (p, info, socket)
    .ContinueWith (antecendent => callback (antecendent));

{
return BeginLaunch (info, callback, null);
}

public static IAsyncResult BeginLaunch (ProcessStartInfo info, AsyncCallback callback, LaunchOptions options)
public static Task<VirtualMachine> BeginLaunch (ProcessStartInfo info, Action<Task<VirtualMachine>> callback, LaunchOptions options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we ok with API breakages?

ListenCallback c = new ListenCallback (ListenInternal);
return c.BeginInvoke (dbg_sock, con_sock, callback, con_sock ?? dbg_sock);

Task<VirtualMachine> t = Task<VirtualMachine>.Run (() => {
Copy link
Contributor

@Therzok Therzok Jan 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above:

return ListenInternal (dbg_sock, conn_sock)
    .ContinueWith (antecendent => callback (antecendent));

@lambdageek
Copy link
Member

Just a heads up - there's some kind of Mono.Debugger.Soft package already published (by us?) in 2017: https://www.nuget.org/packages/Mono.Debugger.Soft/

@jpobst
Copy link
Member

jpobst commented Feb 4, 2021

Any plans to complete this? (At least the part about moving to SDK-style projects.)

We need this for Xamarin.Android in order to build with dotnet, which we need in order to move to .NET5/6.

@jonpryor
Copy link
Member

jonpryor commented Feb 8, 2021

Removing usage of AsyncCallback and AsyncResult, this is not available in netstandard.

This comment does not make sense to me, as public documentation states that these types exist ~everywhere:

Note in particular:

.NET Standard: 1.0, 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 2.0, 2.1

Why do these types need to be removed?

@jonpryor
Copy link
Member

jonpryor commented Feb 8, 2021

The netstandard.dll for net4.5 also contains the forwarders:

% monodis --forward-decls /Library/Frameworks/Mono.framework/Versions/6.12.0/lib/mono/4.5/Facades/netstandard.dll | grep IAsyncRes
.class extern forwarder System.IAsyncResult

% monodis --forward-decls /Library/Frameworks/Mono.framework/Versions/6.12.0/lib/mono/4.5/Facades/netstandard.dll | grep AsyncCallb
.class extern forwarder System.AsyncCallback

@jonpryor
Copy link
Member

jonpryor commented Feb 8, 2021

TaskFactory<T>.FromAsync() can be used to "bundle together" a Begin+End method pair into something that returns a Task.

@Therzok
Copy link
Contributor

Therzok commented Feb 14, 2021

There is a blog post detailing something about this: https://devblogs.microsoft.com/dotnet/migrating-delegate-begininvoke-calls-for-net-core/

Base automatically changed from master to main March 4, 2021 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants