search

microsoft / MSBuildSdks

MSBuild project SDKs
MIT License
449 stars 80 forks source link

Dont use IndexOf as that has Cuture issues #563

Open hknielsen opened 2 months ago

hknielsen commented 2 months ago

Theres currently some culture issues using IndexOf as mentioned in this issue https://github.com/dotnet/msbuild/issues/5502. https://github.com/dotnet/msbuild/issues/5502 fixes it, but its only enabled in a late wave, that we are not depending on yet. This PR uses contains instead, it seem that fixes the issue with Culture for now.

hknielsen commented 2 months ago

Ah Contains dont work in all MSBuild versions, ie the one is used in CI here. @jeffkl do you have any good ideas? What is the IsTraversal property used for? AFAICT its set after ie. Directory.Build.props is imported, so users don't use it to conditionally do logic in those.

jeffkl commented 2 months ago

Its easy for tooling to differentiate between a solution file and a project because they have different extensions. However, its much harder if two projects end in .proj. The idea behind this property is so that tooling can treat a project different if its really just a traversal project. I don't think a ton of tooling has a dependency on it but we certainly can't get rid of the property. I know I used it in SlnGen.

It is set after Directory.Build.props but before the project and Directory.Build.targets. Most tools just evaluate a project and then inspect the properties.

From the issue you linked, it looks more like not properly converting the 0 to a string: https://github.com/dotnet/msbuild/issues/9757

Are you seeing issues because of this call to IndexOf()?

hknielsen commented 2 months ago

From the issue you linked, it looks more like not properly converting the 0 to a string: https://github.com/dotnet/msbuild/issues/9757

Are you seeing issues because of this call to IndexOf()?

Yes in this case its because of the call to IndexOf - we could also make the Property not use that, as its traversal by nature when this props file is imported?

jeffkl commented 2 months ago

I'm a little nervous about changing behavior. Would comparing a string work better?

- <IsTraversal Condition=" '$(IsTraversal)' == '' And $(TraversalProjectNames.IndexOf($(MSBuildProjectFile), System.StringComparison.OrdinalIgnoreCase)) >= 0 ">true</IsTraversal>
+ <IsTraversal Condition=" '$(IsTraversal)' == '' And '$(TraversalProjectNames.IndexOf($(MSBuildProjectFile), System.StringComparison.OrdinalIgnoreCase))' != '0' ">true</IsTraversal>
hknielsen commented 2 months ago

@jeffkl - good idea, that seem to work as well, so im changing the code to do that instead