Remove explicit "src" level from source path construction #304
Conversation
I believe this to be the minimum change to facilitate removal of the duplicated "src" folder level under "src/coreclr" in the runtime repo. Before the removal we'll need to make a coordinated runtime change to pass the source directory as "src/jit" instead of just "jit"; the removal PR should subsequently change the "src/jit" folder as the input argument to jit-format to just "jit". Thanks Tomas
src/jit-format/jit-format.cs
Outdated
| Console.WriteLine("Formatting jit directory."); | ||
| } | ||
| _srcDirectory = "jit"; | ||
| _srcDirectory = "src/jit"; |
There was a problem hiding this comment.
Path.Combine("src", "jit");
There was a problem hiding this comment.
Good point, thanks, fixed in 2nd commit.
BruceForstall
left a comment
BruceForstall
left a comment
There was a problem hiding this comment.
LGTM after fixing Kunal's suggestion.
You can test this locally running jit-format, but the real test requires you to merge it, and then the next dotnet/runtime PR will automatically pick up the change (it clones the jitutils repo)
|
To test, you could locally run |
|
@kunalspathak, @BruceForstall - turns out the jit-format tool was actually missing the definition of the command-line argument to let us specify the source path. I have added it in the latest iteration; could you please take another look when you have a chance? |
I don't think it is needed. We set it correctly to |
|
Hmm, my original thinking was that with the extra parameter the December rollout would be doable by just making a runtime change; without it, the rollout will technically break the build (the formatting leg) and I'll need to "fix it" by making this counterpart change to the jitutils repo. Based on your suggestion I'm just closing this change because there's no point for making it out of sync with the rollout itself - I don't think the two lines of "cleanup" regarding the "src" folder level merit a separate PR, this can be easily fixed as part of the "real" change in lock-step with the rollout. |
As discussed about two weeks back in context of dotnet#304 this change complements the runtime repo PR dotnet/runtime#44973 by fixing the correlated code in jitutils dealing with coreclr paths. Thanks Tomas
3 participants
I believe this to be the minimum change to facilitate removal of
the duplicated "src" folder level under "src/coreclr" in the
runtime repo. Before the removal we'll need to make a coordinated
runtime change to pass the source directory as "src/jit" instead
of just "jit"; the removal PR should subsequently change the
"src/jit" folder as the input argument to jit-format to just "jit".
Thanks
Tomas
Related to: dotnet/runtime#44973
P.S. This is my first chance at a jitutils PR, thanks in advance for your
patience with me; I currently don't know how to test this without
merging in both this change and the envisioned runtime counterpart,
please let me know if you believe there's an easier way.