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

Trim response file lines to prevent forwarding along whitespace-only tokens #26627

Merged
merged 1 commit into from
Aug 5, 2022

Conversation

baronfel
Copy link
Member

After talking with @jonathanpeppers, we discovered that Xamarin-Android creates response files that have leading spaces on the lines. Previous response file handling understood this and did a String.Trim() call on the lines post-comment-stripping, which I lost when I implemented this handling using the new System.CommandLine mechanism for token replacement.

This re-introduces the trimming that should have never left!

@baronfel
Copy link
Member Author

@marcpopMSFT are we able to take this? it fixes some bugs that @jonathanpeppers has identified in MAUI handling.

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Jul 18, 2022

Somehow our tests were passing in a response file with a leading & trailing space characters such as:

 /p:AndroidSdkDirectory="/Users/runner/Library/Android/sdk" 

The leading space character caused:

MSBUILD : error MSB1008: Only one project can be specified.

The trailing space character puts a space in the MSBuild property, and we get other test failures from that.

For now I've removed all the weird spaces from our tests, we'll see how it goes afterward.

@baronfel
Copy link
Member Author

Note that we may not need to rush for 6.0.400 if Jonathan's changes work, since we're very late in that cycle. We could probably retarget to preview7/rc1 instead in that case.

@marcpopMSFT
Copy link
Member

We're still working through getting the final build prepped for 6.0 and we're trying not to let more changes in (ie limit risk). If this can wait, that would be preferred.

@jonathanpeppers
Copy link
Member

Our test run looks green now, thanks.

We have a lot of integration tests that generate projects and build them. It was concerning because all of them were failing before.

jonathanpeppers added a commit to dotnet/android that referenced this pull request Jul 19, 2022
Changes: dotnet/installer@c671341...7a5b36b

Updates: Microsoft.Dotnet.Sdk.Internal: from 6.0.400-preview.22356.7 to 6.0.400-rtm.22364.21

Workarounds for: dotnet/sdk#26627

Remove leading spaces from response file to fix:

    MSBUILD : error MSB1008: Only one project can be specified.

Remove *trailing* spaces from response file to fix the space being included in the property.

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
jonathanpeppers added a commit to dotnet/android that referenced this pull request Aug 1, 2022
Context: dotnet/sdk#26627

This is triggering a bug in the dotnet/sdk.
@jonathanpeppers
Copy link
Member

We started seeing this one in .NET 7 builds here: dotnet/android#7217

@baronfel
Copy link
Member Author

baronfel commented Aug 4, 2022

6.0.4xx is open for merges now, right? @dotnet/dotnet-cli can I get a review on this to get it prepped?

Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

LGTM

@baronfel baronfel merged commit 0c48bfd into dotnet:release/6.0.4xx Aug 5, 2022
jonathanpeppers added a commit to dotnet/android that referenced this pull request Aug 8, 2022
Changes: dotnet/installer@53587f9...8f63969
Changes: dotnet/linker@31a57b5...f09bacf
Changes: dotnet/runtime@aafa910...26a71c6
Changes: dotnet/emsdk@11a9acf...7b2cd1e

Updates:

* Microsoft.Dotnet.Sdk.Internal: from 7.0.100-rc.1.22374.1 to 7.0.100-rc.1.22407.1
* Microsoft.NET.ILLink.Tasks: from 7.0.100-1.22368.1 to 7.0.100-1.22377.1
* Microsoft.NETCore.App.Ref: from 7.0.0-rc.1.22367.4 to 7.0.0-rc.1.22403.8
* Microsoft.NET.Workload.Emscripten.Manifest-7.0.100: from 7.0.0-rc.1.22362.2 to 7.0.0-rc.1.22368.1

~~ Other Changes ~~

* Remove leading/trailing spaces from response file

Context: dotnet/sdk#26627

This is triggering a bug in the dotnet/sdk. We should probably just
leave these this way, as it is weird to have leading/trailing spaces.

* export `DOTNET_CLI_DO_NOT_USE_MSBUILD_SERVER=true` in both
`Makefile` and AzDO `.yaml`.

Context: dotnet/sdk#26965

We should be able to remove this in a future .NET SDK bump.

* Pass along BinaryFormatter warning to BinarySerializableConstraint

Context: https://github.com/dotnet/runtime/blob/197ae4c596553f7e6acb327ca2e31cc00c794c4d/src/libraries/System.Runtime.Serialization.Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatter.Core.cs#L11

Our build was failing with:

    src-ThirdParty/NUnitLite/Constraints/BinarySerializableConstraint.cs(57,17):
    error SYSLIB0011: 'BinaryFormatter.Serialize(Stream, object)' is obsolete: 'BinaryFormatter serialization is obsolete and should not be used. See https://aka.ms/binaryformatter for more information.'
    src-ThirdParty/NUnitLite/Constraints/BinarySerializableConstraint.cs(61,32):
    error SYSLIB0011: 'BinaryFormatter.Deserialize(Stream)' is obsolete: 'BinaryFormatter serialization is obsolete and should not be used. See https://aka.ms/binaryformatter for more information.'

I can add the same `[Obsolete]` messages on
`NUnit.Framework.Constraints.BinarySerializableConstraint` to solve
this error.

* [tests] add one `$(NoWarn)` for `SYSLIB0011`

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants