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

Use architecture specific DOTNET_ROOT variable name #27566

Merged
merged 5 commits into from
Nov 13, 2022

Conversation

tmat
Copy link
Member

@tmat tmat commented Aug 30, 2022

Corrects parsing of runtime identifier added by #19860 and reuses the code in dotnet watch to do the same.

Other than parsing the rid the behavior of #19860 is preserved. However, it doesn't seem consistent with #19743 (comment) in the case the architecture specified in rid is unknown. The implementation in this case sets DOTNET_ROOT variable, while it seems it shouldn't since an unknown architecture doesn't match the current architecture.

Implements dotnet/aspnetcore#35793

@tmat
Copy link
Member Author

tmat commented Sep 6, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmat
Copy link
Member Author

tmat commented Sep 9, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmat tmat changed the title Use architecture specific DOTNET_ROOT veriable name Use architecture specific DOTNET_ROOT variable name Sep 9, 2022
@tmat tmat force-pushed the DOTNET_ROOT branch 3 times, most recently from 9ae8004 to 85fe4fc Compare September 19, 2022 20:42
@tmat tmat changed the base branch from main to release/7.0.1xx-rc2 September 19, 2022 20:42
@tmat tmat marked this pull request as ready for review September 19, 2022 20:42
@tmat
Copy link
Member Author

tmat commented Sep 19, 2022

@mateoatr @vitek-karas PTAL

@vitek-karas
Copy link
Member

However, it doesn't seem consistent with #19743 (comment) in the case the architecture specified in rid is unknown. The implementation in this case sets DOTNET_ROOT variable, while it seems it shouldn't since an unknown architecture doesn't match the current architecture.

That comment was mainly about the case where both architectures are known and the SDK architecture is different from the target architecture - in that case SDK should not set anything, since it doesn't know anything about the target's architecture situation on the machine - thus it should fallback to existing state on the machine to figure it out.
The case where the target architecture is unknown typically means in the "dotnet run" case that it was not specified (I think that can happen), in which case the desire for "dotnet run" is to run the app with the SDK environment - the app should default to SDK's RID in such case basically (which should be the behavior of "dotnet build" as well). I don't know if this is something which actually happens, since I think the msbuild targets will eventually default the target RID to the SDK RID, and thus the command should see the defaulted RID, but maybe it doesn't happen always.

I don't know what is the behavior for "dotnet watch" in this case. The logic should be:

  • If the target is to run on the same architecture as the SDK -> modify the environment to use the SDK's runtime location
  • If the target is to run on a different architecture -> don't modify anything, as the SDK has no clue what that architecture situation is.

I don't feel strongly about the behavior when the target architecture is unknown (I think and hope that it's rare anyway), but since "dotnet run" already defaults to "assume SDK's architecture", then I guess "dotnet watch" should be consistent.

@vitek-karas
Copy link
Member

@MarcoRossignoli as an FYI, since he implemented (and maintains) a similar logic in "dotnet test".

@tmat
Copy link
Member Author

tmat commented Sep 20, 2022

@dotnet/dotnet-cli PTAL

@marcpopMSFT
Copy link
Member

@tmat the linked issue is from a year ago and listed for .net 8. Is this not too risky to take in rc2 at this time?

@tmat
Copy link
Member Author

tmat commented Sep 20, 2022

@marcpopMSFT I'm fine moving to .NET 8. Note though that this also corrects the parsing of RID in dotnet run originally implemented in #19860.

@MarcoRossignoli
Copy link
Member

cc: @Evangelink

@tmat
Copy link
Member Author

tmat commented Sep 21, 2022

Moving to main.

@tmat tmat changed the base branch from release/7.0.1xx-rc2 to main September 21, 2022 19:01
@tmat tmat enabled auto-merge (squash) September 21, 2022 19:02
@tmat
Copy link
Member Author

tmat commented Sep 28, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmat tmat requested review from ericstj, a team and joperezr as code owners October 10, 2022 20:22
@tmat tmat changed the base branch from main to release/7.0.2xx October 10, 2022 20:22
@tmat
Copy link
Member Author

tmat commented Oct 10, 2022

@dsplaisted Rebased to 7.0.2xx

@tmat
Copy link
Member Author

tmat commented Oct 14, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmat
Copy link
Member Author

tmat commented Oct 27, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmat
Copy link
Member Author

tmat commented Nov 9, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dsplaisted
Copy link
Member

@tmat Looks like some of the new EnvironmentVariableNamesTests are failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants