-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Fix helix matrix with built in sdk support #24287
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
Conversation
| @@ -1,5 +1,9 @@ | |||
| # We only want to run full helix matrix on master | |||
| pr: none | |||
| pr: | |||
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.
Revert
|
Marking as ready for review, just needed a few tweaks to work on the additional helix queues |
eng/helix/content/runtests.sh
Outdated
| MAGENTA="\033[0;95m" | ||
|
|
||
| echo "cp -R $HELIX_CORRELATION_PAYLOAD/dotnet $DOTNET_ROOT" | ||
| cp -R $HELIX_CORRELATION_PAYLOAD/dotnet $DOTNET_ROOT |
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.
Two points here:
- Why copy from the correlation payload instead of just running from there?
- Not in the scope of this change but this script looks like it could be improved using $HELIX_WORKITEM_ROOT to get the working directory.
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 tried installing the runtime to the work item root, and leaving the SDK in the payload, but the SDK wouldn't be found by the runtime when I split them up. Ideally the helix sdk would enable installing a specific version of the runtime and sdk at the same time. Yeah I'll clean up the DIR stuff to just use the workitem root 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.
Was it the other way around ie. the SDK (where dotnet should be found in the $PATH) couldn't find the runtimes❔ @dsplaisted @wli3 is there an environment variable that can be used to find runtime directories that aren't under where dotnet is found❔
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 do you mean by "SDK wouldn't be found by the runtime"? Why runtime need to find SDK? (SDK has dependency on runtime, not the other way around)
If you set DOTNET_ROOT, the exe produced by dotnet build will use that runtime instead
eng/helix/content/runtests.sh
Outdated
| MAGENTA="\033[0;95m" | ||
|
|
||
| echo "cp -R $HELIX_CORRELATION_PAYLOAD/dotnet $DOTNET_ROOT" | ||
| cp -R $HELIX_CORRELATION_PAYLOAD/dotnet $DOTNET_ROOT |
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.
Was it the other way around ie. the SDK (where dotnet should be found in the $PATH) couldn't find the runtimes❔ @dsplaisted @wli3 is there an environment variable that can be used to find runtime directories that aren't under where dotnet is found❔
|
Interesting, so symlinking the sdk 'almost' works, looks like dotnet-watch and templates might be relying on mixed bits, where they are crashing/failing now. Trying to see if I can figure out exactly what these two tests are doing differently |
|
Filed #24825 to track if we can fix dotnet-watch and templates tests to not depend on the runtimes included in the sdk and only rely on the latest runtime. |
|
Sym linking the rest of the sdk included runtimes unblocks the templates and dotnet watch tests, my guess is they do something weird with how they package their tests to pickup a dependency from sdk bits that all the rest of our tests do, but everything is green finally |
|
@HaoK looks like this PR might contain the fix for the Helix troubles currently plaguing master. Looks like you're blocked for want of an approving review so I'm going to go ahead and re-request from those who've already made comments. |
|
I think this is a cleaner direct fix for the path issue instead: #24968 |
|
The path issue isn't directly related to us, its dotnet-install that's modifying the system path, so hopefully just specifying -nopath will stop that behavior. Since we are still invoking dotnet-install to pickup runtime bits, this PR won't mitigate the problem entirely (just half of it since we still need to install the version of the runtime we need) |
| echo "Symlink : ln -s $HELIX_CORRELATION_PAYLOAD/dotnet/shared/Microsoft.AspNetCore.App/* $DOTNET_ROOT/shared/Microsoft.AspNetCore.App/" | ||
| ln -s $HELIX_CORRELATION_PAYLOAD/dotnet/shared/Microsoft.AspNetCore.App/* $DOTNET_ROOT/shared/Microsoft.AspNetCore.App/ | ||
| echo "Symlink : ln -s $HELIX_CORRELATION_PAYLOAD/dotnet/shared/Microsoft.NETCore.App/* $DOTNET_ROOT/shared/Microsoft.NETCore.App/" | ||
| ln -s $HELIX_CORRELATION_PAYLOAD/dotnet/shared/Microsoft.NETCore.App/* $DOTNET_ROOT/shared/Microsoft.NETCore.App/ |
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.
Curious: What about dotnet/host/, dotnet/packs/, and dotnet/templates/❔ Did tests work because they're not found using $DOTNET_ROOT❔
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 started with symlinking everything, but it didn't look like we needed all of the other stuff, so I got rid of it... I figure its better to be explicit so we know exactly what we depend on
|
I am added as a reviewer. But I don't have the context of this PR. If you need my review let's have a chat. |
Investigating docker issues with switching to helix sdk support
cc @MattGal