Skip to content

Many references to Common/src/**/Interop code have incorrect casing in the path #28257

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

Closed
dagood opened this issue Dec 21, 2018 · 5 comments
Closed
Milestone

Comments

@dagood
Copy link
Member

dagood commented Dec 21, 2018

The wrong paths I found were all in Common/src/CoreLib/Interop/Windows and Common/src/Interop/Windows. It seems this hasn't been causing a problem yet because they're always built on Windows, which uses a case-insensitive file system.

I hit this because I'm working on getting -allconfigurations to compile on Linux (dotnet/source-build#210), and the casing mismatches cause failures.

I was able to add this target to fix up the Compile items (this also shows what the common casing problems are):

 # In src/Directory.Build.targets
+  <Target Name="FixInteropWindowsKernel32PathCasing"
+          BeforeTargets="BeforeCompile"
+          Condition="'@(Compile)' != ''">
+    <ItemGroup>
+      <_PathToFix
+        Include="Common/src/CoreLib/Interop/Windows/kernel32"
+        FixPath="Common/src/CoreLib/Interop/Windows/Kernel32" />
+      <_PathToFix
+        Include="Common/src/CoreLib/Interop/Windows/advapi32"
+        FixPath="Common/src/CoreLib/Interop/Windows/Advapi32" />
+      <_PathToFix
+        Include="Common/src/Interop/Windows/Kernel32"
+        FixPath="Common/src/Interop/Windows/kernel32" />
+    </ItemGroup>
+
+    <ItemGroup>
+      <_PotentialBadItem
+        Include="@(Compile)"
+        Path="%(_PathToFix.Identity)"
+        Fix="%(_PathToFix.FixPath)" />
+      <_BadInteropItem
+        Include="@(_PotentialBadItem)"
+        Condition="$([System.String]::new('%(Identity)').Contains('%(Path)'))"
+        Fixed="$([System.String]::new('%(Identity)').Replace('%(Path)', '%(Fix)'))" />
+      <Compile Remove="@(_BadInteropItem)" />
+      <Compile Include="@(_BadInteropItem -> '%(Fixed)')" />
+    </ItemGroup>
+  </Target>

I used a target because I didn't want to deal with merge conflicts, but it would probably be better to fix the Compile items through the repo. That target is based on 57358b7 (v3.0.0-preview.18571.3).

dagood referenced this issue in dagood/corefx Dec 21, 2018
Use a task rather than replacing the original lines in the projects to reduce the chance of merge conflicts.

Tracked by https://github.com/dotnet/corefx/issues/34211
@danmoseley
Copy link
Member

@dagood I'd rather not submit a temporary hack. Does this work? https://github.com/dotnet/corefx/compare/master...danmosemsft:casing?expand=1

I would rather you submit it, as it would take a long time for me to set up a Linux machine and biuld all configurations and I'm guessing I need fixes you have on your machine.

I have no idea how to stop this regressing. Maybe change all these paths to consistent casing.

@dagood
Copy link
Member Author

dagood commented Dec 22, 2018

Agreed about the fix, I have this small note but I guess it's pretty tucked in there. 🙂

but it would probably be better to fix the Compile items through the repo.

If I hit more casing issues in the future, I'll just submit a PR. Thanks for the fix!

@danmoseley
Copy link
Member

@dagood if your experiment becomes official I think you should plan on adding the build system check so it fails in Windows.

@dagood
Copy link
Member Author

dagood commented Jan 2, 2019

Thought about that some, and checking for it on Windows actually has an interesting edge case... you can't actually tell what the "correct" case is on Win without asking Git, since Git doesn't (can't?) correct case differences on case-insensitive filesystems. If CI deletes and recreates the repo every time, we can check in CI, but I don't know if that always happens or if it just does cleaning steps in some cases. (It could also cause problems for devs.) I don't know how common this would actually be, but I don't want to introduce machine state dependent failures/flakiness.

Maybe the check could be run conditionally if Git says the casing in general is correct, but then things could slip through and fail later CI/rolling builds. We could run Git commands to figure out if the case is correct (instead of checking on the filesystem), but running one git Exec per Compile item would surely slow down the build. (Maybe this could be batched?) Maybe there's a Git command to fix up casing to what's tracked? I can't find one. I don't see a straightforward way to make this reliable.

I'm not sure this is worth solving before we get an allconfigurations Linux leg in CI here (which would implicitly check case correctness anyway). dotnet/source-build#379 would do that.

@jkotas
Copy link
Member

jkotas commented Jan 2, 2019

I'm not sure this is worth solving before we get an allconfigurations Linux leg in CI here

+1 I suspect that there is number of other issues that can break Linux, but that are not caught by the Windows build. If we want allconfigurations build the work, the best way to check it is to just build it.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants