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

[msbuild/tests] add test for custom framework setup via NuGet package. #1992

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

Conversation

atsushieno
Copy link
Contributor

It proves incomplete but kind of working solution for #1880 .

The original idea (and what once worked) was to only extend an MSBuild
property for ResolveSdks task. But after a couple of changes it does not
work anymore, as there had been a handful of SDK resolution changes.

Since it's not really safe to depend on those properties, we should rather
provide a complete set of framework assemblies (which means, packages
containing both v1.0 and latest) to get app still build.

@atsushieno atsushieno requested a review from dellis1972 as a code owner July 23, 2018 17:04
@atsushieno atsushieno force-pushed the custom-framework-test branch 2 times, most recently from e9cd2ff to 05e8b1b Compare July 24, 2018 05:50
@atsushieno atsushieno added the do-not-merge PR should not be merged. label Jul 25, 2018
@atsushieno
Copy link
Contributor Author

There is a problem in the package; it bundles both v9.0 and v1.0 assemblies, but v1.0 contains mscorlib.dll which should not be separate from the mono runtime. When we build apks, then it will result in inconsistent runtime and BCLs, which could work depending on how fresh the package is and in sync with the runtime in xamarin-android, but more likely could not (due to version mismatch between mono runtime and mscorlib).

The fix should be either to get around TargetFrameworkRootPath limitation, or dump this idea.

@jonpryor
Copy link
Member

"The perfect is the enemy of the good."

In some ways I'd prefer that we figure out how to not have the v1.0 assemblies in the NuGet package -- presumably by providing the directory within the NuGet package as an additional parameter to the AndroidVersions constructor, in the <ResolveSdks/> task -- but should this PR be held up for that change?

How "perfect" does this need to be, when most of this PR is about updating our unit test infrastructure to support such a NuGet package...

I'm inclined to take this PR as-is.

@dellis1972?

public IList<Import> Imports { get; private set; }
public IList<Import> ImportsPrepended { get; private set; }
public IList<Import> ImportsAppended { get; private set; }
[Obsolete ("Use PostImports")]
Copy link
Contributor

Choose a reason for hiding this comment

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

PostImports does not exist.. should it be ImportsAppended ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, yes!

@@ -157,6 +161,15 @@ public virtual List<ProjectResource> Save (bool saveProject = true)
});
}

Func<Import,ProjectResource> create_import_reference = import => new ProjectResource () {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jonpryor should this be a local function? Do we prefer that other a normal method now?

@atsushieno
Copy link
Contributor Author

This PR is not sufficient to prove my ideal concept actually works, but it is rather because my nuget package is bogus and shouldn't be used. A "good" PR would look almost identical, but to get such a package we need some working solution for custom framework setup. This is not what works now.

The idea of giving additional framework directory was almost identical to what I showed at #1880 (there is a diff on the issue comment), but msbuild somehow did not resolve multiple framework paths to the expected one I specified when msbuild resolves TargetFrameworkRootPath. That's the blocker.

It proves incomplete but kind of working solution for dotnet#1880 .

The original idea (and what once worked) was to only extend an MSBuild
property for ResolveSdks task. But after a couple of changes it does not
work anymore, as there had been a handful of SDK resolution changes.

Since it's not really safe to depend on those properties, we should rather
provide a complete set of framework assemblies (which means, packages
containing both v1.0 and latest) to get app still build.
@atsushieno atsushieno force-pushed the custom-framework-test branch from 05e8b1b to 7209646 Compare July 30, 2018 13:30
@atsushieno
Copy link
Contributor Author

There seems still a way to get custom framework assembly referenced and build as expected, but is hacky and likely fragile - by having explicit HintPath to Mono.Android.dll. This means mono.android.jar will be resolved from the framework path, which means inconsistent with Mono.Android.dll.

At this state this is all what I can do.

Base automatically changed from master to main March 5, 2021 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge PR should not be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants