-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Convert fallback path of GetCommandLineArgs to managed #70608
Changes from 5 commits
39d2f90
bc6540a
9711a33
e36103c
aaa5def
03d582a
a3b3db5
01eeb9e
4ff89e1
9ebedfe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Runtime.InteropServices; | ||
|
||
internal static unsafe partial class Interop | ||
{ | ||
internal static partial class Kernel32 | ||
{ | ||
[LibraryImport(Libraries.Kernel32, EntryPoint = "GetCommandLineW")] | ||
internal static partial char* GetCommandLine(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Runtime.InteropServices; | ||
|
||
internal static unsafe partial class Interop | ||
{ | ||
internal static partial class Shell32 | ||
{ | ||
[LibraryImport(Libraries.Shell32, EntryPoint = "CommandLineToArgvW")] | ||
internal static partial char** CommandLineToArgv(char* lpCommandLine, int* pNumArgs); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
using System; | ||
jkotas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
using System.IO; | ||
using System.Reflection; | ||
using TestLibrary; | ||
using Xunit; | ||
|
||
namespace CommandLineTest | ||
{ | ||
class Program | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer this to follow the Removing the namespace, making the class [Fact]
public void Validate_CommandLineArgsSet() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test does not really need to be under src/tests. It can be under libraries (and using remote executor). It is a test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered that under xunit we may have much less control of the actual command line of the process. Can xunit be run in-process by Visual Studio? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AaronRobinsonMSFT What's your thought about the expected value here? I'm afraid the only thing we can test is not empty, which looks not enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In general, I agree. However, the libraries teams have traditionally made an effort to call every API even if it is only to verify it always throws or returns nothing. It does make for some odd tests, but is helpful in ensuring the API works enough to invoke. More to the point, I think it is already done https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime.Extensions/tests/System/Environment.GetCommandLineArgs.cs. Once I update CoreShim to be more useful we can create the more robust suite I think we all want. |
||
{ | ||
int Main() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be marked |
||
{ | ||
// Currently native command line is only implemented on CoreCLR Windows | ||
if (!TestLibrary.Utilities.IsMonoRuntime && !TestLibrary.Utilities.IsNativeAot) | ||
{ | ||
// Clear the command line args set for managed entry point | ||
var field = typeof(Environment).GetField("s_commandLineArgs", BindingFlags.Static | BindingFlags.NonPublic); | ||
Assert.NotNull(field); | ||
field.SetValue(null, null); | ||
|
||
string[] args = Environment.GetCommandLineArgs(); | ||
if (OperatingSystem.IsWindows()) | ||
{ | ||
// The command line should be "corerun assemblyname.dll" for coreclr test | ||
Assert.Equal(2, args.Length); | ||
Assert.Equal("corerun", Path.GetFileNameWithoutExtension(args[0])); | ||
Assert.Equal(typeof(Program).Assembly.GetName().Name, Path.GetFileNameWithoutExtension(args[1])); | ||
} | ||
} | ||
|
||
return 100; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<OutputType>Exe</OutputType> | ||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
</PropertyGroup> | ||
<ItemGroup> | ||
<Compile Include="CommandLineTest.cs" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<ProjectReference Include="$(TestSourceDir)Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" /> | ||
</ItemGroup> | ||
</Project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only other use of SegmentCommandLine is in ildasm that can be eliminated by just using the arguments passed into the main method. It would allow us to delete the
SegmentCommandLine
method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ilasm and ildasm could share the same
main()
, that would also cleanup a lot of code indirection. They are logically doing the same thing (initialize PAL on Unix, and switch over command line args etc.), but ilasm's main implementation is much neater than that of ildasm's.(in a larger picture, corehost and corerun are also candidate of this consolidations, currently they both have their own PAL)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I mentioned this, but I'm not sure if they should be in one PR.
Ideally, many
main
s can share more code, which is some how out of scope.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is infinite amount of cleanup one can do in ilasm/ildasm. I would rather look into C# rewrite than trying to cleanup the existing C/C++ code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkoritzinsky and I have talked about a ilasm/ildasm rewrite for a myriad of reasons. There are many concerns with that but I agree effort to clean-up both code bases is low priority.