Enable sourcelink in source-build #76685
Conversation
|
Tagging subscribers to this area: @hoyosjs Issue DetailsFixes: dotnet/source-build#2883 This is one part of the fix, the other parts are in Arcade and Installer repo. Runtime change is independent and should be merged before the Installer change.
|
|
Infra change only, so this is tell mode. @crummel @MichaelSimons @lbussell can you please provide a code review sign-off? |
Directory.Build.props
Outdated
| @@ -247,6 +247,7 @@ | |||
| <!-- Disable source link when building locally. --> | |||
| <DisableSourceLink Condition="'$(DisableSourceLink)' == '' and | |||
| '$(ContinuousIntegrationBuild)' != 'true' and | |||
| '$(DotNetBuildFromSource)' != 'true' and | |||
There was a problem hiding this comment.
Would it be better to explicitly specify this property in the source-build configuration vs a sourcebuild condition here?
There was a problem hiding this comment.
The benefit is it utilizes the existing mechanism for source-build and reduces the source-build specific checks throughout the code.
There was a problem hiding this comment.
Would it be better to explicitly specify this property in the source-build configuration vs a sourcebuild condition here?
Makes sense - moving it there.
src/coreclr/runtime-prereqs.proj
Outdated
| @@ -6,7 +6,7 @@ | |||
| <RuntimeVersionFile>$(ArtifactsObjDir)runtime_version.h</RuntimeVersionFile> | |||
| <NativeSourceLinkFile>$(ArtifactsObjDir)native.sourcelink.json</NativeSourceLinkFile> | |||
| <VerifySourceLinkFileExists>false</VerifySourceLinkFileExists> | |||
| <VerifySourceLinkFileExists Condition="'$(ContinuousIntegrationBuild)' == 'true' and '$(DotNetBuildFromSource)' != 'true'">true</VerifySourceLinkFileExists> | |||
| <VerifySourceLinkFileExists Condition="'$(ContinuousIntegrationBuild)' == 'true'">true</VerifySourceLinkFileExists> | |||
There was a problem hiding this comment.
Just for mitigating risk, I'd remove changes here. It only applies to windows (native sourcelink).
There was a problem hiding this comment.
This is applicable to Linux as well - NativeSourceLinkFile points to a file that exists in Linux build, so we should check for file existence.
There was a problem hiding this comment.
It will never use it though. It's only passed to MSVC here
There was a problem hiding this comment.
It will never use it though. It's only passed to MSVC here
Interesting. I might be missing when the file gets generated, but it did get created on Linux and had the following content. Should the file even be created on Linux?
{"documents":{"/src/source.online/src/runtime/artifacts/source-build/self/src/*":"https://raw.githubusercontent.com/nikolamilosavljevic/runtime/056cf9bd4c52f7bb8fff74bc47d9c6d2562c3e9c/*"}}
There was a problem hiding this comment.
yeah, the file gets created always. It's used by native only on Windows. It gets used for managed projects mostly.
There was a problem hiding this comment.
Thanks! Since this is useful for Windows only, it makes sense to remove it on source-build and revert the change I proposed.
There was a problem hiding this comment.
Based on this discussion wouldn't it be better to change this to be a windows specific check? e.g. only enabled for windows CI? The point is this really shouldn't be a source-build specific check. I understand however if the desire is to limit changes this late in the release cycle.
|
This change also has an implication: sourcelink will work as long as there is no patch applied to cs files. Otherwise, those files wll be seen as non-matching and won't get loaded. Not a huge thing, but mostly an FYI that I expect to be a rare narrow scenario. |
Yep we are aware of that. We are pushing to eliminate all patches for the 7.0 release. |
ghost
locked as resolved and limited conversation to collaborators
Fixes: dotnet/source-build#2883
This is one part of the fix, the other parts are in Arcade and Installer repo. Runtime change is independent and should be merged before the Installer change.