December infra rollout - remove duplicated 'src' from coreclr subrepo (src/coreclr/src becomes src/coreclr)

[trylek]

@ViktorHofer - surprisingly this turns out to be much easier than I feared ;-).

FYI @dotnet/runtime-infrastructure

[ViktorHofer]

cc @jkotas

[trylek]

OK, I have finally gotten to a mostly functional PR with the exception of the Linux formatting job. The problem with this job is that it uses the jit_format.cs source from the jitutils repo that has the extra "src" level hardcoded in several places, e.g. here:

https://github.com/dotnet/jitutils/blob/4e9ef57fb0803ac6c27f3240964f48951b742c07/src/jit-format/jit-format.cs#L411

I think that the cleanest way forward is by making a preparatory change against the jitutils repo to simplify the path constructions by separating command-line arguments for CoreCLR root and for artifacts root (adding an ArtifactsRoot on top of the existing CoreCLRRoot); this separation enables passing the extra "src" level as parameter from the format.py script and modifying the December roll-out change to just remove this level.

@kunalspathak, you seem to be a frequent contributor to the jitutils repo and you've been recently fixing the jit-format source to cater for Anirudh's "Windows_NT" -> "Windows" rename. Can you please comment on whether my suggestion makes sense and how should I go about the PR process in the jitutils repo I'm not yet too familiar with or recommend someone else for this task? Also, is there any extra process necessary to make the updated jit-format tool available to the runtime pipeline? I'm also wondering whether there are any extra users of the source beyond just the Linux formatting pipeline in the runtime PR, because in such case the proposed change would need to be coordinated with them.

[kunalspathak]

OK, I have finally gotten to a mostly functional PR with the exception of the Linux formatting job. The problem with this job is that it uses the jit_format.cs source from the jitutils repo that has the extra "src" level hardcoded in several places, e.g. here:

https://github.com/dotnet/jitutils/blob/4e9ef57fb0803ac6c27f3240964f48951b742c07/src/jit-format/jit-format.cs#L411

I think that the cleanest way forward is by making a preparatory change against the jitutils repo to simplify the path constructions by separating command-line arguments for CoreCLR root and for artifacts root (adding an ArtifactsRoot on top of the existing CoreCLRRoot); this separation enables passing the extra "src" level as parameter from the format.py script and modifying the December roll-out change to just remove this level.

@kunalspathak, you seem to be a frequent contributor to the jitutils repo and you've been recently fixing the jit-format source to cater for Anirudh's "Windows_NT" -> "Windows" rename. Can you please comment on whether my suggestion makes sense and how should I go about the PR process in the jitutils repo I'm not yet too familiar with or recommend someone else for this task?

That sounds good to me. We can just update the format-job.yml parameter passed to format.py to include src and do what you are proposing inside jit-format.cs file. @BruceForstall , do you see any problem with that? Currently the format-job.yml is only triggered for Linux but we need to fix it for Windows as well. I tried doing it recently in #45015 but there were some other failures that need to be ironed out before we get there.

Also, is there any extra process necessary to make the updated jit-format tool available to the runtime pipeline?

I don't think so.

I'm also wondering whether there are any extra users of the source beyond just the Linux formatting pipeline in the runtime PR, because in such case the proposed change would need to be coordinated with them.

To my knowledge, there is no extra users of the source (and @BruceForstall can correct me). However, we also do similar "src" hardcoding in https://github.com/dotnet/runtime/blob/master/src/coreclr/scripts/superpmi.py which might need to change because we use it to trigger https://github.com/dotnet/runtime/blob/master/eng/pipelines/coreclr/superpmi.yml job.

[trylek]

Thanks Kunal for your detailed explanation! Just to follow up on my initial suggestion, after going over the code in jit-format.cs in more detail I think there's an even simpler way to go about this - just dropping the "src" level from the two places constructing the source path and passing "src\jit" instead of just "jit" as the source directory. The problem with my original suggestion is that we would in fact need three paths as the C# file also uses the direct folder src/coreclr (without the src subfolder) in one place.

[CarolEidt]

@trylek can you articulate the motivation for this change?

[trylek]

@CarolEidt - this change basically repays pre-existing debt as the equivalent transformation was carried out for the libraries and installer subrepo as part of the runtime repo consolidation. For coreclr we were unable to do it at that time due to the fact that we still had tests under the src\coreclr folder. Now that I moved the tests out of the coreclr folder, we no longer need the second "src" folder level and we can put the internal subrepo organization on par with the other subrepos. @ViktorHofer may have additional motivational comments as he's kept motivating me to make this change for quite a while ;-).

[BruceForstall]

While aesthetically I approve (and asked multiple times during the repo merge why we didn't do this), it seems like it will cause a lot of (temporary?) pain for potentially not much real benefit? E.g., it's annoying to have the source history be harder to access.

[trylek]

Thanks Bruce for your feedback. While it's unfortunate that a plain GitHub limitation (poor tracking of moved files) hampers repo innovation in terms of code cleanups, I concur that from a purely practical perspective all cleanup regarding file moves is basically busywork and the extra annoyance in history tracking can easily tip the scales against making it. And while I'd argue that the "aesthetics" may ultimately translate into easier developer ramp-up, orientation and subsequently productivity over time, I'm not sufficiently emotionally attached to this change to push it if the majority consensus is that it's just pain without any benefit; I'd be interested in additional people's opinions on the subject.

[danmoseley]

Doesn't history follow renames, if we do it right?

[jkotas]

Doesn't history follow renames, if we do it right?

It depends on which tool you use to browse the history. For example, tgit has "follow renames" check box that makes it easy.

[danmoseley]

It looks like Github history does not pass --follow in History view, eg
https://github.com/dotnet/runtime/blob/master/src/tests/GC/LargeMemory/largeobject.cs only shows history back to its move. However by clicking blame you can go further and view history from one of those commits.

There is a Chrome extension which claims to join up history.

[ViktorHofer]

I'm in favor of this change as it presumably improves the developer inner loop (less nesting = less typing) middle / long term. We did something similar when we renamed the mscorlib folder to System.Private.CoreLib and lost the Web UI history with it. I don't think we should hold back on improvements because of tooling issues (GH history web view not using - - follow), especially when workarounds (browser extension) or alternatives (git log --follow) are available.

We could recommend folks to install certain extensions for the foreseeable future until there's again sufficient history in the Web UI. Ideally we would have done this as part of the consolidation when history was mutated anyway but at that point we unfortunately weren't ready for this.

[ViktorHofer]

I tentatively added this to next Monday's rollout and just sent out a mail. Please let me know if you have more concerns regarding this change that we should discuss before merging.

[ViktorHofer]

OK I checked the git --name-status of your second commit and everything besides these modifications look good now:

D	src/coreclr/src/.nuget/_.pdb
D	src/coreclr/src/inc/obj/i386/dummy
D	src/coreclr/src/vm/.vscode/c_cpp_properties.json

Were those deletions intentional?

[ViktorHofer]

Perfect, looks good now: git show --pretty="" --name-status e8665e82320ea1c417fa5fc11593bdb05ba2d15d.

@trylek unfortunately you need to resolve two conflicts :(

[trylek]

OK, the change seems to be working modulo the known Formatting linux x64 leg to be fixed by the Jitutils change dotnet/jitutils#308. If I'm about to proceed with this change in a timely manner for the (current) December infra rollout time, I first need an approval on the PR without which I cannot merge the change in; after that I plan to cycle "rebase-rerun" until I hit a state without pending conflicts.

[ViktorHofer]

Verified that the file modifications are correct. After merging this in, let's send out a mail to the team with instructions how to update PRs that are affected by this.

[benaadams]

.sln file also needs updating as it uses relative paths #45738

[EgorBo]

I wish we could fix GitHub history back, this is how all coreclr files now look like when I click "History":

[ViktorHofer]

I agree... Here's one of the browser extensions that might help: https://chrome.google.com/webstore/detail/git-history-browser-exten/laghnmifffncfonaoffcndocllegejnf?hl=en.

[EgorBo]

I agree... Here's one of the browser extensions that might help: https://chrome.google.com/webstore/detail/git-history-browser-exten/laghnmifffncfonaoffcndocllegejnf?hl=en.

Thanks for the link but it doesn't work for me, still shows a single commit: https://github.githistory.xyz/dotnet/runtime/blob/master/src/coreclr/System.Private.CoreLib/src/System/Object.CoreCLR.cs

Same happens with TortoiseGIT
But works for git log --follow Object.CoreCLR.cs

[kunalspathak]

The AzDo has an option to show the history pre-rename.

[imhameed]

Same happens with TortoiseGIT

TortoiseGit has a "Follow Renames" option in the "Walk Behavior" submenu for the log viewer: https://puu.sh/GVukm/aaf86ffb06.png
There's also a "Follow renames" option in the settings dialog for TortoiseGitBlame: https://puu.sh/GVund/c244facf9e.png

[ViktorHofer]

Uff must have mixed that browser extension up with another one. @danmosemsft I think you mentioned that you know about a browser extension that enables history with follow renames?

[danmoseley]

This one, but I didn't try it (I try to avoid browser extensions) https://github.com/staff0rd/github-follow-extension

[jkoritzinsky]

I tested that one out and it works.