Skip to content

Enable sourcelink in source-build #76685

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.

Sign up for GitHub

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

Merged
NikolaMilosavljevic merged 4 commits into from
Oct 7, 2022
Merged

Enable sourcelink in source-build #76685

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7d64ec5
Do not disable sourcelink in source-build
NikolaMilosavljevic Oct 4, 2022
c76c18f
Verify sourcelink file during source-build
NikolaMilosavljevic Oct 4, 2022
9e9c484
Enable sourcelink in SourceBuild by default
NikolaMilosavljevic Oct 5, 2022
a37531f
Native sourcelink file is only used on Windows
NikolaMilosavljevic Oct 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@
<!-- Disable source link when building locally. -->
<DisableSourceLink Condition="'$(DisableSourceLink)' == '' and
'$(ContinuousIntegrationBuild)' != 'true' and
'$(DotNetBuildFromSource)' != 'true' and
Copy link
Member

@MichaelSimons MichaelSimons Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to explicitly specify this property in the source-build configuration vs a sourcebuild condition here?

Copy link
Member

@MichaelSimons MichaelSimons Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benefit is it utilizes the existing mechanism for source-build and reduces the source-build specific checks throughout the code.

Copy link
Member Author

@NikolaMilosavljevic NikolaMilosavljevic Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to explicitly specify this property in the source-build configuration vs a sourcebuild condition here?

Makes sense - moving it there.

'$(OfficialBuildId)' == ''">true</DisableSourceLink>
</PropertyGroup>

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/runtime-prereqs.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

@hoyosjs hoyosjs Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for mitigating risk, I'd remove changes here. It only applies to windows (native sourcelink).

Copy link
Member Author

@NikolaMilosavljevic NikolaMilosavljevic Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is applicable to Linux as well - NativeSourceLinkFile points to a file that exists in Linux build, so we should check for file existence.

Copy link
Member

@hoyosjs hoyosjs Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will never use it though. It's only passed to MSVC here

runtime/eng/native/configurecompiler.cmake

Line 81 in 03f16f1

if(EXISTS ${CLR_SOURCELINK_FILE_PATH})

Copy link
Member Author

@NikolaMilosavljevic NikolaMilosavljevic Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will never use it though. It's only passed to MSVC here

runtime/eng/native/configurecompiler.cmake

Line 81 in 03f16f1

if(EXISTS ${CLR_SOURCELINK_FILE_PATH})

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/*"}}

Copy link
Member

@hoyosjs hoyosjs Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the file gets created always. It's used by native only on Windows. It gets used for managed projects mostly.

Copy link
Member Author

@NikolaMilosavljevic NikolaMilosavljevic Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Since this is useful for Windows only, it makes sense to remove it on source-build and revert the change I proposed.

Copy link
Member

@MichaelSimons MichaelSimons Oct 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<AssemblyName>.NET Runtime</AssemblyName>
</PropertyGroup>

Expand Down