-
Notifications
You must be signed in to change notification settings - Fork 5.3k
implement test for Long Paths support in ProcessModule.FileName #46685
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
Conversation
* CS7013 exceeds the maximum length allowed in metadata for > 255 chars * CS2012: Cannot open ... for writing -- 'The filename, directory name, or volume label syntax is incorrect.
|
Tagging subscribers to this area: @eiriktsarpalis Issue DetailsTo unblock #33505
|
|
My only thought about this is that adding a new assembly to the build slows it down -- I know we've been doing this for some trimming tests, and some other older ones but I guess we should reserve it for where we really have to. Is there not some existing assembly that we could copy to this location instead? (Such as another one of the assemblies we build for testing purposes) |
We could use some other .dll, but it would be harder to guarantee that it's not used (so hasn't been loaded yet and we can copy it somewhere and load from file) |
|
Would something like this be too hacky? if (!AppDomain.CurrentDomain.GetAssemblies().All(assy => !assy.FullName.Contains("System.Data")))
throw new Exception();
string location = Path.GetDirectoryName(Assembly.GetAssembly(typeof(string)).Location);
string assy = Path.Combine(location, "System.Data.dll");
// copy it , load it |
|
@danmosemsft It would be a little bit hacky and I am not sure how adding a dependency to any @eiriktsarpalis what is your opinion on this? |
|
@ViktorHofer @safern We need to test a scenario, where given We have two options:
What is the recommended option from the CI point of view? |
|
Building and loading an additional assembly is fine from a performance standpoint. @adamsitnik I would go with option a and introduce a new test helper assembly. How do you want to load the assembly? Dynamically or via a reference from the test assembly? In case dynamically, you can add a ProjectReference to the new test helper project and set ReferenceOutputAssembly=false to false, i.e. https://github.com/microsoft/MSBuildSdks/blob/1a1e144dd2d5116a280b3f87e1d4ff49c30619b4/src/Traversal/Sdk/Sdk.props#L41. |
src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs
Outdated
Show resolved
Hide resolved
jozkee
left a comment
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.
LGTM.
carlossanlop
left a comment
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.
Left some comments.
src/libraries/Common/tests/LongName/ClassDefinedInAssemblyWithAVeryLongName.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Viktor Hofer <[email protected]>
ViktorHofer
left a comment
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.
LGTM from an infra perspective
To unblock #33505