Skip to content

[xibuild] Handle "incorrectly" cased msbuild property names #6202

Merged
spouliot merged 3 commits intodotnet:masterfrom
radical:fix-xibuild-case-issue
Jun 4, 2019
Merged

[xibuild] Handle "incorrectly" cased msbuild property names #6202
spouliot merged 3 commits intodotnet:masterfrom
radical:fix-xibuild-case-issue

Conversation

@radical
Copy link
Copy Markdown
Member

@radical radical commented Jun 3, 2019

radical added 2 commits June 3, 2019 16:22
msbuild property names are case insensitive. While generating the custom
app.config, in `SetToolsetProperty(..)` we try to update the property if
it already exists. But the name lookup was case sensitive, thus causing
the lookup to fail, resulting in two entries for the same property name
differing only in case. Eg. `MSBuildSDKsPath` vs `MSBuildSdksPath`.

Fixed to ignore case.

Fixes mono/mono#14765 .
@radical radical requested review from rolfbjarne and spouliot June 3, 2019 20:30
@radical
Copy link
Copy Markdown
Member Author

radical commented Jun 3, 2019

/cc @rainersigwald - sanity check:)

Copy link
Copy Markdown
Contributor

@spouliot spouliot left a comment

Choose a reason for hiding this comment

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

minor


var valueAttribute = toolsets.SelectSingleNode ($"property[@name='{name}']/@value");
// MSBuild property names are case insensitive
var valueAttribute = toolsets.SelectSingleNode ($"property[translate(@name, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz')='{name.ToLower()}']/@value");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ToLowerInvariant ?

@monojenkins
Copy link
Copy Markdown
Contributor

Build failure
Build failed or was aborted

Build succeeded
API Diff (from stable)
🔥 Failed to compare API and create generator diff 🔥
    ** Error: Working directory isn't clean:
    HEAD detached at d3a04aa
    Untracked files:
    (use "git add ..." to include in what will be committed)
    
    tests/bcl-test/BCL tests group 1.csproj
    tests/bcl-test/BCL tests group 2.csproj
    tests/bcl-test/BCL tests group 3.csproj
    tests/bcl-test/BCL tests group 4.csproj
    tests/bcl-test/BCL tests group 6.csproj
    tests/bcl-test/Mac OS X BCL tests group 1-mac-full.csproj
    tests/bcl-test/Mac OS X BCL tests group 1-mac-modern.csproj
    tests/bcl-test/Mac OS X BCL tests group 2-mac-full.csproj
    tests/bcl-test/Mac OS X BCL tests group 2-mac-modern.csproj
    tests/bcl-test/Mac OS X BCL tests group 3-mac-full.csproj
    tests/bcl-test/Mac OS X BCL tests group 3-mac-modern.csproj
    tests/bcl-test/Mac OS X BCL tests group 4-mac-full.csproj
    tests/bcl-test/Mac OS X BCL tests group 4-mac-modern.csproj
    tests/bcl-test/generated/
    tests/bcl-test/mscorlib-mac-full.csproj
    tests/bcl-test/mscorlib-mac-modern.csproj
    tests/bcl-test/mscorlib.csproj
    
    nothing added to commit but untracked files present (use "git add" to track)
    Search for Comparing API & creating generator diff in the log to view the complete log.

@monojenkins
Copy link
Copy Markdown
Contributor

Build failure
Build succeeded
API Diff (from stable)
🔥 Failed to compare API and create generator diff 🔥
    ** Error: Working directory isn't clean:
    HEAD detached at 6f4f58e
    Untracked files:
    (use "git add ..." to include in what will be committed)
    
    tests/bcl-test/BCL tests group 1.csproj
    tests/bcl-test/BCL tests group 2.csproj
    tests/bcl-test/BCL tests group 3.csproj
    tests/bcl-test/BCL tests group 4.csproj
    tests/bcl-test/BCL tests group 6.csproj
    tests/bcl-test/Mac OS X BCL tests group 1-mac-full.csproj
    tests/bcl-test/Mac OS X BCL tests group 1-mac-modern.csproj
    tests/bcl-test/Mac OS X BCL tests group 2-mac-full.csproj
    tests/bcl-test/Mac OS X BCL tests group 2-mac-modern.csproj
    tests/bcl-test/Mac OS X BCL tests group 3-mac-full.csproj
    tests/bcl-test/Mac OS X BCL tests group 3-mac-modern.csproj
    tests/bcl-test/Mac OS X BCL tests group 4-mac-full.csproj
    tests/bcl-test/Mac OS X BCL tests group 4-mac-modern.csproj
    tests/bcl-test/generated/
    tests/bcl-test/mscorlib-mac-full.csproj
    tests/bcl-test/mscorlib-mac-modern.csproj
    tests/bcl-test/mscorlib.csproj
    
    nothing added to commit but untracked files present (use "git add" to track)
    Search for Comparing API & creating generator diff in the log to view the complete log.
🔥 Test run failed 🔥

Test results

1 tests failed, 93 tests passed.

Failed tests

  • xammac tests/Mac/Release (all optimizations): Failed (Test run failed.)

@radical
Copy link
Copy Markdown
Member Author

radical commented Jun 3, 2019

MonoTouchFixtures.CoreMidi.MidiThruConnectionTests.ConnectionCreateTest: midi connection error
Expected: True
But was: False

MonoTouchFixtures.CoreMidi.MidiThruConnectionTests.FindTest: connections should not be null
Expected: not null
But was: null

MonoTouchFixtures.CoreMidi.MidiThruConnectionTests.GetSetParamsTest: midi connection error
Expected: True
But was: False

Are these expected/known? They don't look related, based on the errors. But please correct me if I'm wrong and I'll take a look.

@mandel-macaque
Copy link
Copy Markdown
Contributor

mandel-macaque commented Jun 3, 2019

@radical we had a similar issue, but was closed: https://github.com/xamarin/maccore/issues/658

Does not look related to the PR.

@spouliot
Copy link
Copy Markdown
Contributor

spouliot commented Jun 4, 2019

@mandel-macaque re-open the issue and link to it - then it can be merged (and likely backported)

@spouliot spouliot merged commit 55c4073 into dotnet:master Jun 4, 2019
@spouliot
Copy link
Copy Markdown
Contributor

@monojenkins backport to d16-2

@monojenkins
Copy link
Copy Markdown
Contributor

@spouliot backporting to d16-2 failed, the patch results in conflicts:

Applying: [xibuild] Handle "incorrectly" cased msbuild property names
Applying: [xibuild] Use the commonly used casing for `MSBuildSDKsPath` property
Using index info to reconstruct a base tree...
M	tools/xibuild/Main.cs
Falling back to patching base and 3-way merge...
Auto-merging tools/xibuild/Main.cs
CONFLICT (content): Merge conflict in tools/xibuild/Main.cs
error: Failed to merge in the changes.
Patch failed at 0002 [xibuild] Use the commonly used casing for `MSBuildSDKsPath` property

Please backport manually!

spouliot added a commit to spouliot/xamarin-macios that referenced this pull request Jun 11, 2019
rolfbjarne pushed a commit to rolfbjarne/macios that referenced this pull request Jun 19, 2019
)

msbuild property names are case insensitive. While generating the custom
app.config, in `SetToolsetProperty(..)` we try to update the property if
it already exists. But the name lookup was case sensitive, thus causing
the lookup to fail, resulting in two entries for the same property name
differing only in case. Eg. `MSBuildSDKsPath` vs `MSBuildSdksPath`.

Fixed to ignore case.

Fixes mono/mono#14765 .
rolfbjarne added a commit that referenced this pull request Jun 19, 2019
* Bump VSMac to 8.1.0.2742 to fix msbuild issues (#6279)

* Bump VSMac to 8.1.0.2742 to fix msbuild issues

This is required to get the support for the msbuild `ToolsVersion`
change from `15.0` to `Current`.

* [tests][msbuild] Fix Binding resources test with updated msbuild

Test failure with updated msbuild and vsmac 8.1:

```
Xamarin.iOS.Tasks.NativeReferencesNoEmbedding("iPhone").ShouldNotUnnecessarilyRebuildBindingProject(True)
     Binding project build did not create package?
  Expected: True
  But was:  False

at Xamarin.iOS.Tasks.NativeReferencesNoEmbedding.ShouldNotUnnecessarilyRebuildBindingProject (System.Boolean framework) [0x000a0] in <74b8f7d8a53e40109916d305bb4d7403>:0
at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&)
at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo cul
ture) [0x0006a] in <0519fa732e8845b6a809ce9180f541db>:0
```

The test builds the project multiple times. Before the 3rd build, the project
file's timestamp is updated and expects that the binding package will be
rebuilt. But it is not, because the target `_CreateBindingResourcePackage`
doesn't depend on that project file. So, add that to the target inputs.

* [nuget] Use xibuild to run nuget

Fix errors seen during `nuget restore` for tests:

```
Users/builder/jenkins/workspace/xamarin-macios-pr-builder/tests/xammac_tests/xammac_tests.csproj(213,3): error MSB4024: The imported project file "/Library/Frameworks/Mono.framework/External/xbuild/Xamarin/Mac/Xamarin.Mac.CSharp.targets" could not be loaded. Could not find file "/Library/Frameworks/Mono.framework/External/xbuild/Xamarin/Mac/Xamarin.Mac.CSharp.targets"
```

* [xibuild] Fix incorrect mscorlib.dll being used (#6068)

* [xibuild] Fix incorrect mscorlib.dll being used

The `GuiUnit_NET_4_5` project, when built with `xibuild` uses the wrong `mscorlib.dll`.

From #5760 (comment) :

```
- mscorlib.dll is being used from mono/4.5 and the other system assemblies are from mono/4.5-api
- GuiNet* project is built with xibuild

What is happening here is:

	xibuild sets[1] `SetToolsetProperty ("TargetFrameworkRootPath", FrameworksDirectory + Path.DirectorySeparatorChar);`
	which points to `/Library/Frameworks/Mono.framework/Versions/Current/lib/mono/xbuild-frameworks`.
	This causes $(FrameworkPathOverride) to be set[2] to `/Library/Frameworks/Mono.framework/Versions/Current/lib/mono/xbuild-frameworks/.NETFramework/v4.5`,
	but that doesn't have a mscorlib.dll, so it gets reset[3] to /Library/Frameworks/Mono.framework/Versions/5.22.0/lib/mono/4.5/.

	If we don't set TargetFrameworkRoothPath, then we get `FrameworkPathOverride = /Library/Frameworks/Mono.framework/Versions/5.22.0/lib/mono/4.5-api`,
	causing `_ExplicitReference=/Library/Frameworks/Mono.framework/Versions/5.22.0/lib/mono/4.5-api/mscorlib.dll`(correct one) to be used.
```

Fixes #5760

1. https://github.com/xamarin/xamarin-macios/blob/master/tools/xibuild/Main.cs#L209
2. https://github.com/mono/msbuild/blob/xplat-master/src/Tasks/Microsoft.Common.CurrentVersion.targets#L79
3. https://github.com/mono/msbuild/blob/xplat-master/src/Tasks/Microsoft.Common.CurrentVersion.targets#L84

* Revert "Workaround #5760 in generator csproj"

This reverts commit 9bd927b.

The previous commit for xibuild removes the need for this.

* [xibuild] Handle "incorrectly" cased msbuild property names  (#6202)

msbuild property names are case insensitive. While generating the custom
app.config, in `SetToolsetProperty(..)` we try to update the property if
it already exists. But the name lookup was case sensitive, thus causing
the lookup to fail, resulting in two entries for the same property name
differing only in case. Eg. `MSBuildSDKsPath` vs `MSBuildSdksPath`.

Fixed to ignore case.

Fixes mono/mono#14765 .
mandel-macaque pushed a commit that referenced this pull request Jul 19, 2019
msbuild property names are case insensitive. While generating the custom
app.config, in `SetToolsetProperty(..)` we try to update the property if
it already exists. But the name lookup was case sensitive, thus causing
the lookup to fail, resulting in two entries for the same property name
differing only in case. Eg. `MSBuildSDKsPath` vs `MSBuildSdksPath`.

Fixed to ignore case.

Fixes mono/mono#14765 .
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.

MSBuildSDKsPath error in XI with latest 2019-02 Mono/msbuild package

6 participants