Skip to content

Commit

Permalink
Making all of the utility classes private, especially the async-await…
Browse files Browse the repository at this point in the history
… syntax support shims for netfx40, so that to hide them from the compiler and prevent ambiguity when compiling for netfx45+ (gh-20).

Added a testbed usage which demonstrated the problem and got fixed.
  • Loading branch information
hypersw committed Apr 30, 2016
1 parent ce6e2fc commit 857eb7c
Show file tree
Hide file tree
Showing 8 changed files with 21 additions and 8 deletions.
2 changes: 1 addition & 1 deletion ConEmuWinForms/Util/AsyncTaskMethodBuilder!1.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

namespace System.Runtime.CompilerServices
{
public struct AsyncTaskMethodBuilder<TResult>
internal struct AsyncTaskMethodBuilder<TResult>
{
private TaskCompletionSource<TResult> _tasker;

Expand Down
2 changes: 1 addition & 1 deletion ConEmuWinForms/Util/CommandLineBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace Microsoft.Build.Utilities
/// -- Functional double quotes (for example to handle spaces) are best put around both name and value
/// in switches like /Dname=value.
/// </summary>
public sealed class CommandLineBuilder

This comment has been minimized.

Copy link
@vbjay

vbjay Jul 24, 2016

Why was this class made internal? It breaks https://github.com/gitextensions/gitextensions/blob/master/GitUI/UserControls/ConsoleEmulatorOutputControl.cs#L76 when trying to upgrade nuget package.

This comment has been minimized.

Copy link
@ArsenShnurkov

ArsenShnurkov Sep 10, 2016

Did you created corresponding issue in GitExtensions? What is it number? I have the same error - 857eb7c#commitcomment-18974700
and created an issue in this repository - #29

This comment has been minimized.

Copy link
@jbialobr

jbialobr Sep 12, 2016

GitExtensions is .net 4.0 targeted and Microsoft.Build.Utilities.Core requires 4.5.
We can copy CommandLineBuilder.cs from MS as it is done here or make the conemu-inside a submodule and include it to GitExt solution to have a better control over the source code.

This comment has been minimized.

Copy link
@ArsenShnurkov

ArsenShnurkov Sep 12, 2016

4.6 in it's current source - https://github.com/Microsoft/msbuild/blob/7992abc02c44260f374c1a8cd781a1d13fba17c4/src/nuget/Microsoft.Build.Utilities.Core.nuspec#L25
One can also create a separate package for CommandLineBuilder (for 4.0 framework, with custom .csproj and .nuspec) which could be reused in both projects.

This comment has been minimized.

Copy link
@vbjay

vbjay via email Sep 12, 2016

This comment has been minimized.

Copy link
@ArsenShnurkov

ArsenShnurkov Sep 13, 2016

I compared this CommandLineBuilder and one from msbuild. This one contains less code, for example it doesn't depend on ITaskItem, so it can be compiled separately.

But I didn't understood, how it is related to "async" problem, they are 2 sets of unrelated code. So why not to make just CommandLineBuilder public?

internal sealed class CommandLineBuilder
{
public static readonly string License =
@"
Expand Down
2 changes: 1 addition & 1 deletion ConEmuWinForms/Util/IAsyncStateMachine.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

namespace System.Runtime.CompilerServices
{
public interface IAsyncStateMachine
internal interface IAsyncStateMachine
{
void MoveNext();

Expand Down
2 changes: 1 addition & 1 deletion ConEmuWinForms/Util/ICriticalNotifyCompletion.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

namespace System.Runtime.CompilerServices
{
public interface ICriticalNotifyCompletion : INotifyCompletion
internal interface ICriticalNotifyCompletion : INotifyCompletion
{
[SecurityCritical]
void UnsafeOnCompleted(Action continuation);
Expand Down
2 changes: 1 addition & 1 deletion ConEmuWinForms/Util/INotifyCompletion.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

namespace System.Runtime.CompilerServices
{
public interface INotifyCompletion
internal interface INotifyCompletion
{
void OnCompleted(Action continuation);
}
Expand Down
2 changes: 1 addition & 1 deletion ConEmuWinForms/Util/TaskAwaiter!1.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

namespace System.Threading.Tasks
{
public struct TaskAwaiter<TResult> : INotifyCompletion
internal struct TaskAwaiter<TResult> : INotifyCompletion
{
private readonly bool _isctx;

Expand Down
2 changes: 1 addition & 1 deletion ConEmuWinForms/Util/TaskAwaiter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

namespace System.Threading.Tasks
{
public struct TaskAwaiter : INotifyCompletion
internal struct TaskAwaiter : INotifyCompletion
{
private readonly bool _isctx;

Expand Down
15 changes: 14 additions & 1 deletion Tests/ControlDllTestbed/Testbed.cs
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,23 @@ private static Form RenderView(Form form)
if(result == DialogResult.Cancel)
return;
ConEmuSession session = conemu.Start(new ConEmuStartInfo() {ConsoleProcessCommandLine = "choice", IsEchoingConsoleCommandLine = true, WhenConsoleProcessExits = result == DialogResult.Yes ? WhenConsoleProcessExits.KeepConsoleEmulatorAndShowMessage : WhenConsoleProcessExits.CloseConsoleEmulator, ConsoleProcessExitedEventSink = (sender, args) => MessageBox.Show($"Your choice is {args.ExitCode} (powered by startinfo event sink).")});
session.WaitForConsoleProcessExitAsync().ContinueWith(task => MessageBox.Show($"Your choice is {task.Result.ExitCode} (powered by wait-for-exit-async)."));
#pragma warning disable 4014
ShowMessageForChoiceAsync(session);
#pragma warning restore 4014
};

return form;
}

/// <summary>
/// This method checks that the async-await compiler syntax is not prevented in netfx45+ projects due to the shim types present in the conemu assembly (https://github.com/Maximus5/conemu-inside/issues/20).
/// </summary>
private static async Task ShowMessageForChoiceAsync(ConEmuSession session)
{
if(session == null)
throw new ArgumentNullException(nameof(session));
ConsoleProcessExitedEventArgs exitargs = await session.WaitForConsoleProcessExitAsync();
MessageBox.Show($"Your choice is {exitargs.ExitCode} (powered by wait-for-exit-async).");
}
}
}

4 comments on commit 857eb7c

@ArsenShnurkov
Copy link

@ArsenShnurkov ArsenShnurkov commented on 857eb7c Sep 10, 2016

Choose a reason for hiding this comment

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

and after making it "internal" GitExtensions build gives the error:
UserControls/ConsoleEmulatorOutputControl.cs(88,19): error CS0122: `Microsoft.Build.Utilities.CommandLineBuilder' is inaccessible due to its protection level
/usr/lib/mono/gac/ConEmu.WinForms/1.0.0.0__00340228797aafb8/ConEmu.WinForms.dll (Location of the symbol related to previous error)
...
Done building target "CoreCompile" in project "/var/calculate/remote/distfiles/egit-src/gitextensions.git/GitUI/GitUI.csproj".-- FAILED

@hypersw
Copy link
Collaborator Author

@hypersw hypersw commented on 857eb7c Sep 13, 2016

Choose a reason for hiding this comment

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

I've made all the utility code internal, so that it wouldn't show up in your solution and cause ambiguity in code completion etc.
As the cmdline formatter is useful in clients as well, I'd rather make it back public and make the name a bit more unique maybe.
#29

@jbialobr
Copy link

Choose a reason for hiding this comment

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

Consider making ConEmuStartInfo.ConsoleProcessCommandLine of the new type (CommandLineBuilder) instead of string. It will give a hint that the ConsoleProcessCommandLine has to be escaped.

@hypersw
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possible.
This is not 100% correct because the escaping is not formally required, the target process receives the command line string AS IS and can parse it at its own discretion with any escaping rules it likes. Actually, escaping rules do vary from app to app, and the CommandLineBuilder implements some average sane rules.
But it allows to submit unescaped raw text if needed, so would do for all uses.

Please sign in to comment.