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

Fix non-existing language targets restore #186

Merged
merged 5 commits into from
Sep 27, 2019

Conversation

jeromelaban
Copy link
Contributor

@jeromelaban jeromelaban commented Sep 26, 2019

This PR fixes the scenario where building a head project should only build the proper dependency chain and ignore unknown targets (e.g. xamarinios10 on linux).

The current behavior is the following:

/mnt/c/temp/msbe-linux/ClassLibrary1/ClassLibrary1.csproj error MSB4057: The target "_GetRestoreSettingsPerFramework" does not exist in the project. [/mnt/c/temp/msbe-linux/ClassLibrary1/ClassLibrary1.csproj]

This PR also adds validations on Linux and macOS.

For the Linux tests, the MSBuild version installed version in the azure devops hosted agents is not working properly (some missing methods in nuget), so the build needs to rely on a container that has the proper setup applied.

See https://github.com/nventive/docker/blob/master/wasm-build/Dockerfile

@@ -1,54 +1,97 @@
trigger:
Copy link
Collaborator

@clairernovotny clairernovotny Sep 26, 2019

Choose a reason for hiding this comment

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

Be careful with the file encoding. There's an issue where UTF8 with BOM isn't being correctly handled. This file needs to be either ANSI or UTF8 without a BOM. The build error looks like it's tripping up over this.

@jeromelaban
Copy link
Contributor Author

jeromelaban commented Sep 26, 2019 via email

@jeromelaban jeromelaban force-pushed the dev/jela/linux-fix branch 5 times, most recently from 6705a4b to bf92940 Compare September 26, 2019 19:44
@jeromelaban
Copy link
Contributor Author

jeromelaban commented Sep 26, 2019

@onovotny I'm a bit puzzled by the modification I had to make in order for the tests to pass. Do you remember why it would be needed to conditionally include the original language targets ?

It's all in CheckMissing.targets.

@clairernovotny
Copy link
Collaborator

clairernovotny commented Sep 27, 2019

@jeromelaban That import in CheckMissing is intended to only do the import of the default in case the target doesn't exist. Otherwise, the SDK imports it here: https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/sdk/Sdk.targets#L37

Those sdk targets are called here https://github.com/onovotny/MSBuildSdkExtras/blob/f1476ba441d9eacf4152ea04e5cc2e05b97ea6f8/Source/MSBuild.Sdk.Extras/Sdk/Sdk.targets#L15

That said, CheckMissing is called after that point given that it goes:

SDK.target import -> LanguageTargets (like Windows.targets) -> CheckMissing.

However -- and this is big -- in MSBuild 16, because of MSBuildAllProjects being conditional and no longer used, the mechanism no longer works. There's no string/variable that comes from the common targets I can test reliably to see if it has already been imported.

@jeromelaban jeromelaban force-pushed the dev/jela/linux-fix branch 5 times, most recently from 6c3b090 to b3863a3 Compare September 27, 2019 11:14
Fixes an issue where the platform targets are not available (building a netstandard 2.0 project that references a "netstandard2.0;xamarinios10" project on Linux.
@jeromelaban
Copy link
Contributor Author

I see. This is why I'm seeing double imports warnings in the tests. I'll see if I can find a way to detect that, maybe I'll be lucky :)

@jeromelaban
Copy link
Contributor Author

@onovotny It seems that we can rely on CommonTargetsPath to determine if common targets have been included. It fixes both the missing targets issue, and the duplicate imports messages.

@jeromelaban jeromelaban marked this pull request as ready for review September 27, 2019 13:15
@clairernovotny
Copy link
Collaborator

@jeromelaban Nice, not sure how I missed that as I was trying to find something like that!

azure-pipelines.yml Outdated Show resolved Hide resolved
@clairernovotny clairernovotny merged commit 6fc4cc6 into novotnyllc:master Sep 27, 2019
@jeromelaban jeromelaban deleted the dev/jela/linux-fix branch September 27, 2019 20:26
@clairernovotny
Copy link
Collaborator

Thanks for tracking this down and setting up those tests!

I'll get this pushed to NuGet once the master build completes.

@jeromelaban
Copy link
Contributor Author

My pleasure :) We'll be able to add actual 16.3 tests soon.

@clairernovotny
Copy link
Collaborator

16.3 is on the agent now if that helps.

@jeromelaban
Copy link
Contributor Author

I did not notice! That was fast. Then it'll be the turn for macOS and Linux agents.

@clairernovotny
Copy link
Collaborator

Not sure when that will be for those. There was an urgent need for 16.3 given the hard error with .NET Core 3.

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