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

Add limited net11/net30/net35/net403 support (mostly for NuGet packages) #88

Merged
merged 2 commits into from
Oct 9, 2019

Conversation

j3parker
Copy link
Member

@j3parker j3parker commented Oct 9, 2019

This an alternative to #66 (for #59 )

I think this is way less hacky.

@j3parker j3parker requested a review from omsmith October 9, 2019 01:33
name = "OldThing",
srcs = ["Class.cs"],
target_frameworks = ["net11"],
include_stdrefs = False,
Copy link
Member Author

@j3parker j3parker Oct 9, 2019

Choose a reason for hiding this comment

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

Without this, the error message is:

The dependency @net//:StandardReferences is not compatible with net11!

I think this is a pretty good outcome!

@@ -102,9 +110,13 @@ FrameworkCompatibility = {
"netstandard2.1": ["netstandard2.0"],

# .NET Framework
"net11": [],
Copy link
Member Author

Choose a reason for hiding this comment

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

So the idea here is that we never "fill in" these frameworks with compatible ones (e.g. net20 -> net30) but these can be used to populate the other direction (net30 -> net40). The precedence is the natural order.

Maybe we don't need to do this. I wanted to prevent @net//:System.Linq from existing though, because that's funky. It does mean it'd be extra hard to build net30 things with csharp_library, but I don't know that we should really be worried about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, true, having net20 provide the standard references for everything else wouldn't be amazing.

"net40": ["net20"],
"net45": ["net40", "netstandard1.1"],
"net30": [],
"net35": [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just fill out these edges? As you've shown, we're technically supporting building with them, just without the availability of the framework.

Does not having "net35": ["net30"] mean you can't build a net45 library, that depends on a net35 nuget, which itself depends on a net30 nuget?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was freaked out about accidentally creating @net35//:System.Linq. I don't feel strongly about it though.

Yeah it might. Ok I'll do the normal thing.

@j3parker
Copy link
Member Author

j3parker commented Oct 9, 2019

You know, maybe import_multiframework_library could just signal what things it didn't want filled in or something. It seems like a minor issue though.

@omsmith
Copy link
Contributor

omsmith commented Oct 9, 2019

Yeah, I was going to say, I think we just want something like a no_version_smearing or whatever flag on the rules, which we can use for the framework references.

This demonstrates that the framework selection for packages is working
right.
@j3parker
Copy link
Member Author

j3parker commented Oct 9, 2019

The only hits for net11 in the action graph, as desired:

2019-10-09T01:52:37.8286901Z action 'Compiling OldThing'
2019-10-09T01:52:37.8286996Z   Mnemonic: CSharpCompile
2019-10-09T01:52:37.8287088Z   Target: //import_nuget_package:OldThing
2019-10-09T01:52:37.8287328Z   Configuration: k8-fastbuild
2019-10-09T01:52:37.8287408Z   ActionKey: 49a152d61ad2aeb19e89ed71a6b79bdb
2019-10-09T01:52:37.8288306Z   Inputs: [external/ExamplePackageFolder/lib/net11/SomePackage.dll, external/csharp-build-tools/tasks/netcoreapp2.1/bincore/csc.dll, external/netcore-runtime-linux/dotnet, import_nuget_package/Class.cs]
2019-10-09T01:52:37.8289121Z   Outputs: [bazel-out/k8-fastbuild/bin/import_nuget_package/bazelout/net11/OldThing.dll, bazel-out/k8-fastbuild/bin/import_nuget_package/bazelout/net11/OldThing.pdb, bazel-out/k8-fastbuild/bin/import_nuget_package/bazelout/net11/OldThing.ref.dll]
2019-10-09T01:52:37.8289545Z   Command Line: (exec external/netcore-runtime-linux/dotnet \
2019-10-09T01:52:37.8289886Z     external/csharp-build-tools/tasks/netcoreapp2.1/bincore/csc.dll \
2019-10-09T01:52:37.8290010Z     /noconfig \
2019-10-09T01:52:37.8290286Z     /unsafe- \
2019-10-09T01:52:37.8290560Z     /checked- \
2019-10-09T01:52:37.8290667Z     /nostdlib+ \
2019-10-09T01:52:37.8290774Z     /utf8output \
2019-10-09T01:52:37.8290879Z     /deterministic+ \
2019-10-09T01:52:37.8290967Z     /filealign:512 \
2019-10-09T01:52:37.8291243Z     /nologo \
2019-10-09T01:52:37.8291331Z     /highentropyva+ \
2019-10-09T01:52:37.8291419Z     /warn:0 \
2019-10-09T01:52:37.8291514Z     /target:library \
2019-10-09T01:52:37.8291604Z     /langversion:7.3 \
2019-10-09T01:52:37.8291694Z     /debug+ \
2019-10-09T01:52:37.8291937Z     /optimize- \
2019-10-09T01:52:37.8292158Z     '/define:TRACE;DEBUG' \
2019-10-09T01:52:37.8292250Z     /debug:portable \
2019-10-09T01:52:37.8292541Z     /out:bazel-out/k8-fastbuild/bin/import_nuget_package/bazelout/net11/OldThing.dll \
2019-10-09T01:52:37.8292856Z     /refout:bazel-out/k8-fastbuild/bin/import_nuget_package/bazelout/net11/OldThing.ref.dll \
2019-10-09T01:52:37.8293167Z     /pdb:bazel-out/k8-fastbuild/bin/import_nuget_package/bazelout/net11/OldThing.pdb \
2019-10-09T01:52:37.8293284Z     /r:external/ExamplePackageFolder/lib/net11/SomePackage.dll \
2019-10-09T01:52:37.8293385Z     import_nuget_package/Class.cs \
2019-10-09T01:52:37.8293479Z     /d:NETFRAMEWORK \
2019-10-09T01:52:37.8293574Z     /d:NET11)
2019-10-09T01:52:37.8293615Z 

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