Skip to content
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

Fixup jitutils to work with the dotnet/runtime repo #244

Merged
merged 2 commits into from Jan 4, 2020
Merged

Conversation

@tannergooding
Copy link
Member

tannergooding commented Jan 2, 2020

This resolves #230 #243

This does not provide any switch to continue working on dotnet/coreclr, as I assume there wont be any significant changes and devs needing to test changes can always use an older snapshot of jitutils (such as commit 7e6c68b or prior).

@@ -198,11 +198,11 @@ private void validate()
if (!Directory.Exists(_rootPath))
{
// If _rootPath doesn't exist, it is an invalid path
_syntaxResult.ReportError("Invalid path to coreclr directory. Specify with --coreclr");
_syntaxResult.ReportError("Invalid path to runtime directory. Specify with --coreclr");

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jan 2, 2020

Author Member

I kept it as --coreclr for back-compat. We could also expose a --runtime option or only expose --runtime and remove --coreclr.

We could also just rename it to --root or --root-path to make it clearer and not tie it to the repo name 😄

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 2, 2020

Member

I vote to change it to --runtime but also leave --coreclr (hidden, if possible) for back compat. -c is used by src\coreclr\tests\scripts\format.py in the CI formatting jobs.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jan 3, 2020

Author Member

Do we want to shortcut --runtime as -r?

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 3, 2020

Member

No strong opinion, but maybe yes if it doesn't conflict with anything else?

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jan 3, 2020

Author Member

It does not conflict, I've updated it to take -r or --runtime and the help message on c|coreclr to indicate it exists for back-compat and to use r|runtime moving forward (I didn't see any option to make it hidden).

Copy link
Member

BruceForstall left a comment

Thanks for doing this!

@@ -198,11 +198,11 @@ private void validate()
if (!Directory.Exists(_rootPath))
{
// If _rootPath doesn't exist, it is an invalid path
_syntaxResult.ReportError("Invalid path to coreclr directory. Specify with --coreclr");
_syntaxResult.ReportError("Invalid path to runtime directory. Specify with --coreclr");

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 2, 2020

Member

I vote to change it to --runtime but also leave --coreclr (hidden, if possible) for back compat. -c is used by src\coreclr\tests\scripts\format.py in the CI formatting jobs.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Jan 2, 2020

CC. @AndyAyersMS since you logged #230

If you feel strongly about supporting both repos, I can add in some kind of switch to support coreclr and runtime; although I think just creating a tag for the rare servicing scenario might be sufficient.

The more interesting reason for both might be doing comparisons between runtime and coreclr; but that would require a switch that works for --base and --diff (and might be more beneficial if done alongside #217).

@BruceForstall

This comment has been minimized.

Copy link
Member

BruceForstall commented Jan 2, 2020

I don't think we need to support the old repos (as you state, you can just check out an older version of jitutils).

@AndyAyersMS

This comment has been minimized.

Copy link
Member

AndyAyersMS commented Jan 3, 2020

Keeping --coreclr aka -c and introducing a new --runtime seems prudent.

Do the CI jobs on coreclr run jit-format (looks like no, at least for 3.1). I suppose there's limited need for back compat.

@BruceForstall

This comment has been minimized.

Copy link
Member

BruceForstall commented Jan 3, 2020

I think we disabled jit-format in coreclr servicing CI. (And if we didn't, we could do that.)

@BruceForstall

This comment has been minimized.

Copy link
Member

BruceForstall commented Jan 3, 2020

Looks great. Ship it!

@tannergooding tannergooding merged commit c17da2f into master Jan 4, 2020
6 checks passed
6 checks passed
WIP Ready for review
Details
jitutils-ci Build #20200103.1 succeeded
Details
jitutils-ci (Linux) Linux succeeded
Details
jitutils-ci (Windows_NT) Windows_NT succeeded
Details
jitutils-ci (macOS) macOS succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.