-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fix casing of files to get closer to ./build.sh -allConfiguration working on linux #35085
Conversation
I was working on this (linux allconfigurations) for source-build! I summarized my latest progress at dotnet/source-build#210 (comment) because I had to stop working on it. The
|
@dagood hope this is not conflicting with anything. In the issue you mention you got the workaround for the build.. I just need to be able to build packages locally. You mentioned to use |
No conflict. It's cool to see someone else trying to do this. 😄
Depending on which packages you want to build, you don't need
I think it would take too long for me to go back and try to figure out what the minimum is to do this. It has likely been broken by later corefx commits, too. You can try taking my WIP branch of source-build (https://github.com/dagood/source-build/tree/corefx-all), building it, and checking the state of the |
@@ -128,7 +128,7 @@ | |||
<Compile Include="$(CommonPath)\Interop\Windows\kernel32\Interop.DeleteVolumeMountPoint.cs"> | |||
<Link>Common\Interop\Windows\Interop.DeleteVolumeMountPoint.cs</Link> | |||
</Compile> | |||
<Compile Include="$(CommonPath)\CoreLib\Interop\Windows\kernel32\Interop.GetLogicalDrives.cs"> | |||
<Compile Include="$(CommonPath)\CoreLib\Interop\Windows\Kernel32\Interop.GetLogicalDrives.cs"> |
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.
Should we instead be consistent and rename this directories to be all lowercase? It is odd to have someones to be lowercase and other uppercase.
@@ -47,11 +47,11 @@ | |||
<Compile Include="$(CommonPath)\CoreLib\Interop\Windows\Kernel32\Interop.FormatMessage.cs"> | |||
<Link>Common\CoreLib\Interop\Windows\Interop.FormatMessage.cs</Link> | |||
</Compile> | |||
<Compile Include="$(CommonPath)\CoreLib\Interop\Windows\advapi32\Interop.RegCloseKey.cs"> | |||
<Compile Include="$(CommonPath)\CoreLib\Interop\Windows\Advapi32\Interop.RegCloseKey.cs"> |
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.
Same here.
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.
which do we prefer?
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.
I would prefer lowercase, and that seem to be the majority of the cases, but I don't know what other people think on this.
cc @jkotas @stephentoub @danmosemsft
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.
The casing of the filename should match the casing of the class
that the file or directory contains.
The interop classes start with upper case: class Kernel32
, class Advapi32
, ... .
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.
The upper case is much more common, e.g.:
Kernel32
1571 hits over 415 files
kernel32
616 hits over 96 files
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.
We should use the upper case, primariliy for the reason @jkotas mentions. We want the code to use PascalCasing for these types, as otherwise it stands out like a sore thumb, and the file names should match the type names.
<Compile Include="$(CommonPath)\Interop\Windows\User32\Interop.MessageBeep.cs"> | ||
<Link>Common\Interop\Windows\User32\Interop.MessageBeep.cs</Link> | ||
<Compile Include="$(CommonPath)\Interop\Windows\user32\Interop.MessageBeep.cs"> | ||
<Link>Common\Interop\Windows\user32\Interop.MessageBeep.cs</Link> |
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.
We should make these upper case. If it's not on the file system, we should rename the directory as well.
911ff2c
to
e98c779
Compare
I've also normalized slashes to use Windows, please let me know if you find any other directories which need renaming |
@safern ping - this PR is super merge conflict prone |
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! Thanks for fixing it!
Let me know if you got any other massive casing changes, I wrote tiny hacky tool for that so it will go quickly in the future |
Match convention introduced by dotnet/corefx#35085
Match convention introduced by dotnet/corefx#35085 Signed-off-by: dotnet-bot <[email protected]>
Match convention introduced by dotnet#35085 Signed-off-by: dotnet-bot <[email protected]>
Match convention introduced by #35085 Signed-off-by: dotnet-bot <[email protected]>
Match convention introduced by dotnet/corefx#35085 Signed-off-by: dotnet-bot <[email protected]>
Match convention introduced by dotnet/corefx#35085 Signed-off-by: dotnet-bot <[email protected]>
Match convention introduced by dotnet/corefx#35085 Signed-off-by: dotnet-bot <[email protected]>
…king on linux (dotnet/corefx#35085) * Rename directories to use CamelCasing * Fix CsProj files to match file names and their casing * fix couple of Compile items which were not in csproj file Commit migrated from dotnet/corefx@8c52600
Match convention introduced by dotnet/corefx#35085 Signed-off-by: dotnet-bot <[email protected]> Commit migrated from dotnet/coreclr@be1b5db
Currently
./build.sh -allConfiguration
is not working on linux.This is fixing one class of issues.
There are still some errors I'm seeing locally but not sure how to fix yet (will be digging a bit more):
Not seeing
netcoreapp-Linux-Debug-x64-aot
in my bin foldercc: @dotnet/dnceng