Skip to content

Condition source-link arguments #11153

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 1 commit into from
Oct 7, 2022
Merged

Condition source-link arguments #11153

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildArcadeBuild.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@
<InnerBuildArgs>$(InnerBuildArgs) /p:SourceBuiltBlobFeedDir=$(SourceBuiltBlobFeedDir)</InnerBuildArgs>

<!-- Work around issue where local clone may cause failure using non-origin remote fallback: https://github.com/dotnet/sourcelink/issues/629 -->
<InnerBuildArgs>$(InnerBuildArgs) /p:EnableSourceControlManagerQueries=false</InnerBuildArgs>
<InnerBuildArgs>$(InnerBuildArgs) /p:EnableSourceLink=false</InnerBuildArgs>
<InnerBuildArgs>$(InnerBuildArgs) /p:DeterministicSourcePaths=false</InnerBuildArgs>
<InnerBuildArgs Condition="'$(DisableSourceLink)' == 'true'">$(InnerBuildArgs) /p:EnableSourceControlManagerQueries=false</InnerBuildArgs>
<InnerBuildArgs Condition="'$(DisableSourceLink)' == 'true'">$(InnerBuildArgs) /p:EnableSourceLink=false</InnerBuildArgs>
Copy link
Member

@MichaelSimons MichaelSimons Oct 7, 2022

Choose a reason for hiding this comment

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

Why both and EnableSourceLink and DisableSourceLink? This is confusing IMO. Would it make sense to toggle EnableSourceControlManagerQueries and DeterministicSourcePaths based on EnableSourceLink and just use EnableSourceLink to control the behavior?

Copy link
Member

@MichaelSimons MichaelSimons Oct 7, 2022

Choose a reason for hiding this comment

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

I guess this DisableSourceLink is something that already exists. 😞

Copy link
Member Author

@NikolaMilosavljevic NikolaMilosavljevic Oct 7, 2022

Choose a reason for hiding this comment

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

In light of this change: https://github.com/dotnet/runtime/pull/76685/files - would it actually make sense to completely remove these 3 lines instead?

DisableSourceLink seems to be a higher-level property, controlling the values of these 3 source-link related properties, like in here:

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/RepositoryInfo.targets

Lines 13 to 17 in 48e76fb

<PropertyGroup Condition="'$(DisableSourceLink)' == 'true'">
<EnableSourceLink>false</EnableSourceLink>
<EnableSourceControlManagerQueries>false</EnableSourceControlManagerQueries>
<DeterministicSourcePaths>false</DeterministicSourcePaths>
</PropertyGroup>

Similarly, in installer repo:
https://github.com/dotnet/installer/blob/e9549ff1d3412a94f101be476cec553f0bf858e8/Directory.Build.props#L35-L37

As a result, we would also remove the same 3 lines from: https://github.com/dotnet/installer/blob/e9549ff1d3412a94f101be476cec553f0bf858e8/src/SourceBuild/tarball/content/ArcadeOverrides/SourceBuildArcadeBuild.targets#L89-L91

Copy link
Member

@MichaelSimons MichaelSimons Oct 7, 2022

Choose a reason for hiding this comment

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

Yes, removing these would be good. They appear completely redundant now.

<InnerBuildArgs Condition="'$(DisableSourceLink)' == 'true'">$(InnerBuildArgs) /p:DeterministicSourcePaths=false</InnerBuildArgs>
</PropertyGroup>

<ItemGroup>
Expand Down