Skip to content

Add long paths support#33030

Merged
RikkiGibson merged 9 commits into
dotnet:masterfrom
RikkiGibson:long-paths
Feb 15, 2019
Merged

Add long paths support#33030
RikkiGibson merged 9 commits into
dotnet:masterfrom
RikkiGibson:long-paths

Conversation

@RikkiGibson
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson commented Jan 31, 2019 •
edited
Loading

Resolves #32804

  • Support long paths in csc
  • Support long paths in other executables (vbc, csi, VBCSCompiler, etc)
    - [ ] Reuse configs where possible between executables
    - [ ] Conditional test for long path support which spawns a csc subprocess
    - [ ] BuildBoss update to check proper configuration of .exe projects

@RikkiGibson RikkiGibson requested a review from a team as a code owner January 31, 2019 23:54
@RikkiGibson
Copy link
Copy Markdown
Member Author

RikkiGibson commented Feb 1, 2019 •
edited
Loading

Executable projects to update include:

src\Tools\Source\RunTests\RunTests.csproj
src\Compilers\CSharp\csc\csc.csproj
src\Compilers\Server\VBCSCompiler\VBCSCompiler.csproj
src\Compilers\VisualBasic\vbc\vbc.csproj
src\Interactive\csi\csi.csproj
src\Tools\BuildBoss\BuildBoss.csproj
src\Tools\CompilerBenchmarks\CompilerBenchmarks.csproj
src\Tools\ILAsm\IlAsmDeploy.csproj
src\Tools\Source\CompilerGeneratorTools\Source\BoundTreeGenerator\CompilersBoundTreeGenerator.csproj
src\Tools\Source\CompilerGeneratorTools\Source\CSharpErrorFactsGenerator\CSharpErrorFactsGenerator.csproj
src\Tools\Source\CompilerGeneratorTools\Source\CSharpSyntaxGenerator\CSharpSyntaxGenerator.csproj
src\Tools\Source\CompilerGeneratorTools\Source\VisualBasicErrorFactsGenerator\VisualBasicErrorFactsGenerator.vbproj
src\Tools\Source\CompilerGeneratorTools\Source\VisualBasicSyntaxGenerator\VisualBasicSyntaxGenerator.vbproj

Should each one of these be manually tested?

Seems like some of these don't have associated App.configs, so maybe they should be filtered out? Shipping tools under Compilers/ and Interactive/ seem more relevant than those under Tools/.

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Feb 1, 2019

@RikkiGibson

Should each one of these be manually tested?

No. I'd focus on testing one or two. That combined with the BuildBoss change to ensure they all have a manifest should be sufficient for covering our bases.

@RikkiGibson RikkiGibson requested review from a team as code owners February 1, 2019 23:29
tmat
tmat reviewed Feb 1, 2019
View reviewed changes
Comment thread src/Compilers/VisualBasic/vbc/App.config Outdated
tmat
tmat reviewed Feb 1, 2019
View reviewed changes
Comment thread src/Compilers/VisualBasic/vbc/app.manifest Outdated
@RikkiGibson RikkiGibson changed the base branch from dev16.1-preview1 to master February 8, 2019 05:43
tmat
tmat reviewed Feb 11, 2019
View reviewed changes
Comment thread eng/config/app.manifest Outdated
@RikkiGibson RikkiGibson changed the title [WIP] Add long paths support Add long paths support Feb 12, 2019
@RikkiGibson
Copy link
Copy Markdown
Member Author

RikkiGibson commented Feb 12, 2019

It's looking prohibitively difficult to test long paths support because the test runner isn't long path aware.

jaredpar
jaredpar reviewed Feb 12, 2019
View reviewed changes
Comment thread src/Tools/BuildBoss/BuildBoss.csproj Outdated
<SignAssembly>false</SignAssembly>
<UseVSHostingProcess>false</UseVSHostingProcess>
<IsShipping>false</IsShipping>
<ApplicationManifest>$(RepositoryEngineeringDir)config\app.manifest</ApplicationManifest>
Copy link
Copy Markdown
Member

@jaredpar jaredpar Feb 12, 2019

Choose a reason for hiding this comment

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

We should update BuildBoss to ensure every project with OutputType of EXE also has an ApplicationManifest entry.

Copy link
Copy Markdown
Member Author

@RikkiGibson RikkiGibson Feb 12, 2019

Choose a reason for hiding this comment

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

Sounds good

Copy link
Copy Markdown
Member

@tmat tmat Feb 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Can we instead set ApplicationManifest property in Imports.targets if the project set OutputType to exe or winexe and the target is .NET Framework? That way we'll have correctness by construction. I don't think the app manifest should be set when targeting .NET Core.

Copy link
Copy Markdown
Member

@tmat tmat Feb 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm thinking this might actually be a good default in Arcade SDK -- if the project does not specify ApplicationManifest (or even in .NET Core SDK). I'll follow up on moving this to Arcade SDK once we have Roslyn change checked in and working.

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Feb 12, 2019

Done with review pass (iteration 4)

chsienki
chsienki reviewed Feb 12, 2019
View reviewed changes
Comment thread eng/targets/Imports.targets
@RikkiGibson
Copy link
Copy Markdown
Member Author

RikkiGibson commented Feb 14, 2019

FYI @tmat @jaredpar @chsienki the change which automatically copies in the manifest for projects with OutputType == Exe and TargetFramework == net472 is complete

@RikkiGibson RikkiGibson merged commit 12f4665 into dotnet:master Feb 15, 2019
@RikkiGibson RikkiGibson deleted the long-paths branch February 15, 2019 22:39
@tmat tmat mentioned this pull request Feb 21, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tmat tmat tmat approved these changes

@jaredpar jaredpar jaredpar approved these changes

@chsienki chsienki chsienki approved these changes

Assignees

No one assigned

Labels

Area-Compilers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@RikkiGibson @jaredpar @tmat @chsienki @jinujoseph