Skip to content

Conversation

@adamsitnik
Copy link
Member

To unblock #33505

 * 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.
@adamsitnik adamsitnik added this to the 6.0.0 milestone Jan 7, 2021
@ghost
Copy link

ghost commented Jan 7, 2021

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

Issue Details

To unblock #33505

Author: adamsitnik
Assignees: -
Labels:

area-System.Diagnostics.Process, test enhancement

Milestone: 6.0.0

@danmoseley
Copy link
Member

danmoseley commented Jan 7, 2021

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)

@adamsitnik
Copy link
Member Author

Is there not some existing assembly that we could copy to this location instead?

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)

@danmoseley
Copy link
Member

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

@adamsitnik
Copy link
Member Author

@danmosemsft It would be a little bit hacky and I am not sure how adding a dependency to any System*.csproj would affect the build time? Could it defeat the purpose of not slowing down the build by adding a new library?

@eiriktsarpalis what is your opinion on this?

@adamsitnik
Copy link
Member Author

@ViktorHofer @safern We need to test a scenario, where given .dll that has not been yet loaded into memory, has a path longer than 260 characters.

We have two options:

  • create a new project, ensure the path is > 260 chars and load the dll. It ensures that the .dll has not been loaded yet (because nobody has been using it so far), but adds a new project.
  • add a reference to a library that the given test suite has not been using so far, ensure the path is > 260 chars, and load it. It might change the project dependencies and build order but does not add a new project.

What is the recommended option from the CI point of view?

@ViktorHofer
Copy link
Member

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.

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Left some comments.

Copy link
Member

@ViktorHofer ViktorHofer left a 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

@adamsitnik adamsitnik merged commit 8b391f6 into dotnet:master Feb 23, 2021
@adamsitnik adamsitnik deleted the processLongPaths branch February 23, 2021 14:41
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants