Skip to content
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.

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

Dont use IndexOf as that has Cuture issues #563

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hknielsen
Copy link
Contributor

Theres currently some culture issues using IndexOf as mentioned in this issue dotnet/msbuild#5502.
dotnet/msbuild#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
Copy link
Contributor Author

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
Copy link
Contributor

jeffkl commented Jun 27, 2024

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: dotnet/msbuild#9757

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

@hknielsen
Copy link
Contributor Author

From the issue you linked, it looks more like not properly converting the 0 to a string: dotnet/msbuild#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
Copy link
Contributor

jeffkl commented Jul 2, 2024

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
Copy link
Contributor Author

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

Using string seem to solve this issue for now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants