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

Make debug command line arguments parsing consistent on Unix platforms #17106

Merged
merged 1 commit into from Feb 27, 2017

Conversation

marek-safar
Copy link
Contributor

Customer scenario

d837ae8 changed default debug format on non-windows platforms but only for explicitly passed full or pdbonly arguments which makes it inconsistent with windows and documentation.

Without this change csc /debug or csc /debug+ still fail with

error CS0041: Unexpected error writing debug information -- 'Windows PDB writer is not available -- could not find Microsoft.DiaSymReader.Native.amd64.dll'

@gafter
Copy link
Member

gafter commented Feb 14, 2017

@tmat This purports to fix a bug you recently introduced.

/cc @dotnet/roslyn-compiler

This probably doesn't belong in the master branch at this time. We'll know in a few days where it should be targeted.

@gafter gafter requested a review from tmat February 14, 2017 01:27
@marek-safar
Copy link
Contributor Author

I'd like to see this to be in next RC so we don't have to patch msbuild to work with csc on non-windows platforms

@jaredpar
Copy link
Member

I'd like to see this to be in next RC so we don't have to patch msbuild to work with csc on non-windows platforms

Unfortunately this won't meet the bar for RC. It will have to go into the 15.1 release instead.

As @gafter pointed out though this likely need to be merged into a different branch than this one. The infra for this is being worked out today / tomorrow. Will report back once we have everything setup.

@@ -60,7 +60,7 @@ public new CSharpCommandLineArguments Parse(IEnumerable<string> args, string bas
bool concurrentBuild = true;
bool deterministic = false; // TODO(5431): Enable deterministic mode by default
bool emitPdb = false;
DebugInformationFormat debugInformationFormat = DebugInformationFormat.Pdb;
DebugInformationFormat debugInformationFormat = PathUtilities.IsUnixLikePlatform ? DebugInformationFormat.PortablePdb : DebugInformationFormat.Pdb;
Copy link
Member

Choose a reason for hiding this comment

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

Can we get some tests for this?

@@ -92,7 +92,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
Dim concurrentBuild As Boolean = True
Dim deterministic As Boolean = False
Dim emitPdb As Boolean
Dim debugInformationFormat As DebugInformationFormat = DebugInformationFormat.Pdb
Dim debugInformationFormat As DebugInformationFormat = If(PathUtilities.IsUnixLikePlatform, DebugInformationFormat.PortablePdb, DebugInformationFormat.Pdb)
Copy link
Member

Choose a reason for hiding this comment

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

Can we get some tests for this?

@AlekseyTs
Copy link
Contributor

Agree with @jaredpar on testing.

@marek-safar
Copy link
Contributor Author

There is a test for this change already which is failing (part of d837ae8) but because you are running tests on windows only nothing is red

@jaredpar
Copy link
Member

@marek-safar ah I see. Yeah right now we're only running the parser tests on Windows. The remainder of the tests are getting moved over in the next month or so.

@marek-safar
Copy link
Contributor Author

@jaredpar could you merge it

@jaredpar
Copy link
Member

@marek-safar need one more sign off for merge.

@tmat can you sign off on this?

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM

@tmat
Copy link
Member

tmat commented Feb 27, 2017

👍

@jaredpar jaredpar merged commit 8d86ccf into dotnet:master Feb 27, 2017
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.

None yet

6 participants