Permit individually settable search paths #7008

Forgind · 2021-11-01T18:49:45Z

Context

AssemblySearchPaths are currently set using a property (not an item because order often matters) in which either the user sets all of them or the user accepts the default. This allows the user to accept some or most of the default without accepting all of it.

It may also be reasonable to add:

<AssemblySearchPaths Condition="'$(CustomAssemblySearchPaths)' != ''">$(AssemblySearchPaths);$(CustomAssemblySearchPaths)</AssemblySearchPaths>

though that would only allow users to add custom paths at the predefined spot, so it may not be too helpful. I'm also unsure if it would be most likely to be used if it's first, last, or just before OutDir.

Fixes dotnet#3784

benvillalobos

LGTM. Friendly reminder that we need these properties inserted in our public facing docs (if i recall correctly, that was the location we agreed on?)

Forgind · 2021-11-10T19:34:53Z

LGTM. Friendly reminder that we need these properties inserted in our public facing docs (if i recall correctly, that was the location we agreed on?)

To both "known MSBuild properties" and our repo-level docs, but we also decided I should put that off until rainersigwald has had a chance to review it, and it's been merged.

rainersigwald

Can you follow this up with an equivalent change in the .NET SDK, which overrides our defaults? https://github.com/dotnet/sdk/blob/5899560935260ff35a9ca601c91c75afbaed1f1d/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.props#L91

Forgind · 2021-11-17T21:46:29Z

Can you follow this up with an equivalent change in the .NET SDK, which overrides our defaults? https://github.com/dotnet/sdk/blob/5899560935260ff35a9ca601c91c75afbaed1f1d/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.props#L91

It looks cleaner as-is than if I were to change it to use the new properties in this PR to me. If I did, I'd have to set several properties instead of just one. Also, it currently sets DesignTimeAssemblySearchPaths (if it's nonempty) to $(AssemblySearchPaths), and that wouldn't work anymore; I'd have to have modify this PR to also set DesignTimeAssemblySearchPaths to $(AssemblySearchPaths) like here if it's nonempty, but that would be changing where that happened and making it happen in non-SDK style projects where it hadn't before.

rainersigwald · 2021-11-17T22:01:58Z

It looks cleaner as-is than if I were to change it to use the new properties in this PR to me. If I did, I'd have to set several properties instead of just one.

What I'm asking for is to respect these new properties in the .NET SDK as well. Otherwise we'll be in a position where we say "you can set AssemblySearchPathUseCandidateAssemblyFiles=false to avoid using CandidateAssemblyFiles, but that only works if you're not using the .NET SDK".

I'd have to have modify this PR to also set DesignTimeAssemblySearchPaths to $(AssemblySearchPaths) like here if it's nonempty

Divergence of DesignTimeAssemblySearchPaths from AssemblySearchPaths worries me.

Forgind · 2021-11-17T22:11:06Z

It looks cleaner as-is than if I were to change it to use the new properties in this PR to me. If I did, I'd have to set several properties instead of just one.

What I'm asking for is to respect these new properties in the .NET SDK as well. Otherwise we'll be in a position where we say "you can set AssemblySearchPathUseCandidateAssemblyFiles=false to avoid using CandidateAssemblyFiles, but that only works if you're not using the .NET SDK".

Oh, makes sense.

I'd have to have modify this PR to also set DesignTimeAssemblySearchPaths to $(AssemblySearchPaths) like here if it's nonempty

Divergence of DesignTimeAssemblySearchPaths from AssemblySearchPaths worries me.

Do you think setting DesignTimeAssemblySearchPaths where I pointed to is too much of a breaking change? It feels pretty small to me, but I really don't know. The only way I can think of to fully avoid any change in behavior is if the SDK checks for all the new properties and sets AssemblySearchPaths then DesignTimeAssemblySearchPaths appropriately, but that's a bit messy.

src/Tasks/Microsoft.Common.CurrentVersion.targets

rainersigwald · 2021-11-23T15:43:53Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseCandidateAssemblyFiles) != 'false'">{CandidateAssemblyFiles}</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseReferencePath) != 'false'">$(AssemblySearchPaths);$(ReferencePath)</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseHintPathFromItem) != 'false'">$(AssemblySearchPaths);{HintPathFromItem}</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseTargetFrameworkDirectory) != 'false'">$(AssemblySearchPaths);{TargetFrameworkDirectory}</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseAssemblyFoldersConfigFileSearchPath) != 'false'">$(AssemblySearchPaths);$(AssemblyFoldersConfigFileSearchPath)</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseRegistry) != 'false'">$(AssemblySearchPaths);{Registry:$(FrameworkRegistryBase),$(TargetFrameworkVersion),$(AssemblyFoldersSuffix)$(AssemblyFoldersExConditions)}</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseAssemblyFolders) != 'false'">$(AssemblySearchPaths);{AssemblyFolders}</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseGAC) != 'false'">$(AssemblySearchPaths);{GAC}</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseRawFileName) != 'false'">$(AssemblySearchPaths);{RawFileName}</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseOutDir) != 'false'">$(AssemblySearchPaths);$(OutDir)</AssemblySearchPaths>


Can you please also add these to the XSD?

Nirmal4G · 2021-12-20T20:55:43Z

Related: #772

Nirmal4G · 2021-12-20T21:03:41Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseAssemblyFolders) != 'false'">$(AssemblySearchPaths);{AssemblyFolders}</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseGAC) != 'false'">$(AssemblySearchPaths);{GAC}</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseRawFileName) != 'false'">$(AssemblySearchPaths);{RawFileName}</AssemblySearchPaths>
+    <AssemblySearchPaths Condition="$(AssemblySearchPathUseOutDir) != 'false'">$(AssemblySearchPaths);$(OutDir)</AssemblySearchPaths>


Sorry for the late review but...

Suggested change

<AssemblySearchPaths Condition="$(AssemblySearchPathUseOutDir) != 'false'">$(AssemblySearchPaths);$(OutDir)</AssemblySearchPaths>

<AssemblySearchPaths Condition="'$(AssemblySearchPath_UseOutDir)' != 'false'">$(AssemblySearchPaths);$(OutDir)</AssemblySearchPaths>

CHANGES

Use quotes to avoid erroring out with a malformed value (with space and illegal symbols) entering the condition.

Wouldn't it better to use BaseProperty_SubOption with an _ acting as a separator to make these properties clearer?

See that we already use this pattern of naming here...

msbuild/src/Tasks/Microsoft.Common.props

Line 181 in 14313d1

<DisableLogTaskParameter_ConvertToAbsolutePath_Path>true</DisableLogTaskParameter_ConvertToAbsolutePath_Path>

Forgind · 2021-12-20T23:25:51Z

Related: #772

Didn't this resolve #772? Am I missing something?

Nirmal4G · 2021-12-21T08:16:23Z

It does but I linked the issue here via the comment above.

Follow-ups from Permit individually settable search paths #7008 (review) Respect opt-outs from #7008 sdk#22719 (comment) Context For multi-level properties, follow the naming pattern similar to BaseProperty_SubOption with an _ underscore character acting as a separator to make these properties clearer. We already follow this pattern in Common props where we have DisableLogTaskParameter_ConvertToAbsolutePath_Pathand friends. msbuild/src/Tasks/Microsoft.Common.props Lines 181 to 183 in 14313d1 <DisableLogTaskParameter_ConvertToAbsolutePath_Path>true</DisableLogTaskParameter_ConvertToAbsolutePath_Path> <DisableLogTaskParameter_FindUnderPath_OutOfPath>true</DisableLogTaskParameter_FindUnderPath_OutOfPath> <DisableLogTaskParameter_RemoveDuplicates_Inputs>true</DisableLogTaskParameter_RemoveDuplicates_Inputs> Changes Made So, Use AssemblySearchPath_Use{Variant} property format to control individual inclusion of different variants of assembly search paths. By default, these properties will be opt-out to maintain back-compat. Notes This change hasn't been shipped yet. So, it's technically not breaking and it's also not too late to change it.

Respect opt-outs from dotnet/msbuild#7008

Permit individually settable search paths

f3343e4

Fixes dotnet#3784

benvillalobos approved these changes Nov 10, 2021

View reviewed changes

Forgind requested a review from rainersigwald November 15, 2021 16:27

rainersigwald approved these changes Nov 17, 2021

View reviewed changes

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Nov 17, 2021

Forgind removed the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Nov 17, 2021

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Nov 22, 2021

Forgind added a commit to Forgind/sdk that referenced this pull request Nov 22, 2021

Respect opt-outs from dotnet/msbuild#7008

e41e0eb

This was referenced Nov 22, 2021

Respect opt-outs from dotnet/msbuild#7008 dotnet/sdk#22719

Merged

Document opt-outs #7063

Closed

Forgind commented Nov 22, 2021

View reviewed changes

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved

Forgind added 2 commits November 22, 2021 09:35

Update src/Tasks/Microsoft.Common.CurrentVersion.targets

1d74268

Document opt-outs

85f22d5

rainersigwald reviewed Nov 23, 2021

View reviewed changes

Add ASP and opt-outs to xsd

8c90d6e

rainersigwald merged commit 14313d1 into dotnet:main Nov 23, 2021

Nirmal4G reviewed Dec 20, 2021

View reviewed changes

Forgind deleted the individual-AssemblySearchPaths branch December 20, 2021 23:25

Forgind mentioned this pull request Dec 21, 2021

Assembly search path should be more configurable to support reproducible builds #772

Closed

Nirmal4G mentioned this pull request Dec 22, 2021

Rename AssemblySearchPathUseXXX to AssemblySearchPath_UseXXX for better readability #7166

Merged

Forgind added a commit to dotnet/sdk that referenced this pull request Jan 14, 2022

Respect opt-outs from dotnet/msbuild#7008 (#22719)

70477d9

Respect opt-outs from dotnet/msbuild#7008

KirillOsenkov mentioned this pull request Jul 12, 2024

Analyzer for "bad" AssemblySearchPaths #10368

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permit individually settable search paths #7008

Permit individually settable search paths #7008

Forgind commented Nov 1, 2021

benvillalobos left a comment

Forgind commented Nov 10, 2021

rainersigwald left a comment

Forgind commented Nov 17, 2021

rainersigwald commented Nov 17, 2021

Forgind commented Nov 17, 2021

rainersigwald Nov 23, 2021

Nirmal4G commented Dec 20, 2021

Nirmal4G Dec 20, 2021 •

edited

Loading

Forgind commented Dec 20, 2021

Nirmal4G commented Dec 21, 2021

	<AssemblySearchPaths Condition="$(AssemblySearchPathUseOutDir) != 'false'">$(AssemblySearchPaths);$(OutDir)</AssemblySearchPaths>
	<AssemblySearchPaths Condition="'$(AssemblySearchPath_UseOutDir)' != 'false'">$(AssemblySearchPaths);$(OutDir)</AssemblySearchPaths>

Permit individually settable search paths #7008

Permit individually settable search paths #7008

Conversation

Forgind commented Nov 1, 2021

Context

benvillalobos left a comment

Choose a reason for hiding this comment

Forgind commented Nov 10, 2021

rainersigwald left a comment

Choose a reason for hiding this comment

Forgind commented Nov 17, 2021

rainersigwald commented Nov 17, 2021

Forgind commented Nov 17, 2021

rainersigwald Nov 23, 2021

Choose a reason for hiding this comment

Nirmal4G commented Dec 20, 2021

Nirmal4G Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Forgind commented Dec 20, 2021

Nirmal4G commented Dec 21, 2021

Nirmal4G Dec 20, 2021 •

edited

Loading