-
Notifications
You must be signed in to change notification settings - Fork 507
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
Allow overriding HOME variable in dotnet remote builds #15171
Conversation
|
||
//In case the XMA dotnet has not been installed yet | ||
if (!File.Exists (dotnetPath)) { | ||
if (File.Exists (dotnetPath)) { | ||
Environment.SetEnvironmentVariable ("DOTNET_XMA_HOME", Path.Combine (sdkRootPath, ".home")); |
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.
What happens if you just set HOME here? If that works, I don't think you need the rest of the changes.
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.
Yeah, I thought about it but I was worried about messing up other things. But the set "should" affect only the current process, so it might work. I'll do a quick try
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.
@rolfbjarne it doesn't work because who needs the HOME override is the dotnet process, not the Build (agent) process
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 could avoid the DOTNET_XMA_HOME and use HOME instead but the environment change on the base tasks would be needed anyways
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.
That's weird, if you set an environment variable for the current process, all child processes should inherit that variable
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.
@rolfbjarne It works, I just updated the PR by changing only the Build agent and setting HOME. I was verifying the wrong bits
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Regarding this:
You'll have to add it to the EnvironmentVariables property (from the base ToolTask class): https://docs.microsoft.com/en-us/dotnet/api/microsoft.build.utilities.tooltask.environmentvariables?view=msbuild-17-netcore
This looks like dead code, I can't see it used anywhere, so you could probably just delete it: xamarin-macios/dotnet/targets/Xamarin.Shared.Sdk.targets Lines 1057 to 1061 in da7dd02
I think it would be required. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
With remote builds, a dedicated dotnet location is being used so the right versioning can be used and managed from VS in Windows. This dedicated dotnet location requires a custom .home location so the donet and nuget caches doesn't conflict with the global installation. We need to consider and use this custom .home location when running dotnet commands remotely so the right caches are used
📋 [PR Build] API Diff 📋API diff (for current PR)ℹ️ API Diff (from PR only) (please review changes) .NETXamarin vs .NETAPI diff (vs stable)✅ API Diff from stable .NETXamarin vs .NETGenerator diff✅ Generator Diff (no change) Pipeline on Agent XAMBOT-1108.Monterey' |
💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻✅ All tests on macOS Mac Catalina (10.15) passed. Pipeline on Agent |
@rolfbjarne is this ready to merge? |
Test failure is unrelated (https://github.com/xamarin/maccore/issues/2558). |
With remote builds, a dedicated dotnet location is being used so the right versioning can be used and managed from VS in Windows. This dedicated dotnet location requires a custom .home location so the donet and nuget caches doesn't conflict with the global installation.
We need to consider and use this custom .home location when running dotnet commands remotely so the right caches are used
This should fix Bug: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1543495