-
Notifications
You must be signed in to change notification settings - Fork 148
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
Fixing up some infrastructure so we can consume the libclang 8.0.0 packages #65
Conversation
@@ -67,7 +67,7 @@ | |||
|
|||
<!-- Package references which are consumed by all projects --> | |||
<ItemGroup> | |||
<PackageReference Include="Microsoft.Net.Compilers.Toolset" IsImplicitlyDefined="true" /> | |||
<PackageReference Include="Microsoft.Net.Compilers.Toolset" IsImplicitlyDefined="true" PrivateAssets="all" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to mark this as PrivateAssets="all"
so that the product packages don't list it as a dependency.
@@ -15,6 +15,11 @@ | |||
<DefineConstants>$(DefineConstants);$(OS)</DefineConstants> | |||
</PropertyGroup> | |||
|
|||
<!-- Settings that allow testing to work by default --> | |||
<PropertyGroup> | |||
<RuntimeIdentifier Condition="'$(RuntimeIdentifier)' == ''">$(NETCoreSdkRuntimeIdentifier)</RuntimeIdentifier> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The libclang package is setup using a runtime.json
file, so tests can't run without a RID
being specified (so the correct libclang` native dependency can be resolved). This sets the default to just be the same as the host SDK.
<PackageReference Update="System.CommandLine.Experimental" Version="0.2.0-alpha.19174.3" /> | ||
<PackageReference Update="xunit" Version="2.3.1" /> | ||
<PackageReference Update="xunit.runner.visualstudio" Version="2.3.1" /> | ||
<PackageReference Update="xunit" Version="2.4.1" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were a few versions old. Upgrading to get perf and other minor benefits.
@@ -0,0 +1,278 @@ | |||
============================================================================== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LICENSE.TXT was taken directly from https://github.com/llvm-mirror/clang
<metadata minClientVersion="2.12"> | ||
<id>libclang</id> | ||
<version>8.0.0</version> | ||
<authors>Microsoft and Contributors</authors> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NuGet lists this as being the package authors/owners, not the binary authors/owners.
@@ -0,0 +1,59 @@ | |||
{ | |||
"runtimes": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLVM supports 12 platforms, of those, .NET Core supports 11 (no PowerPC
support).
"runtime.osx-x64.libclang": "8.0.0" | ||
} | ||
}, | ||
"sles-x64": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The native dll is technically sles.11.3
, but that isn't a valid RID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.NET supports sles.12
as the earliest version
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<OutputType>Exe</OutputType> | ||
<TargetFramework>netcoreapp2.0</TargetFramework> | ||
<TargetFramework>netcoreapp2.1</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
netcoreapp2.0 went EOL (end of life) back in October of 2018
Once this is approved/merged, I can publish the libclang.8.0.0 nuget packages and can submit the PR consuming them. libllvm is going to be a bit more complex as it requires building per RID (which is feasible, it just will take a bit of time 😄). |
packages/libclang/libclang.nuspec
Outdated
<owners>Microsoft and Contributors</owners> | ||
<requireLicenseAcceptance>true</requireLicenseAcceptance> | ||
<license type="expression">Apache-2.0</license> | ||
<projectUrl>https://github.com/llvm-mirror/clang</projectUrl> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The license should definitely match the original binaries, but it might make sense to use the clangsharp project url, instead of theirs. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also the repository url a few lines down...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, looks like NuGet lists projectUrl
as
A URL for the package's home page, often shown in UI displays as well as nuget.org.
repository
is listed as:
Repository metadata, consisting of four optional attributes: type and url (4.0+), and branch and commit (4.6+). These attributes allow you to map the .nupkg to the repository that built it, with the potential to get as detailed as the individual branch or commit that built the package. This should be a publicly available url that can be invoked directly by a version control software. It should not be an html page as this is meant for the computer. For linking to project page, use the projectUrl field, instead.
@mjsabby, what do you think of making projectUrl
be https://github.com/microsoft/ClangSharp
and repository
be: <repository type="git" url="https://github.com/llvm-mirror/clang" branch="release_80" />
?
I think that would satisfy both who created the package and where the sources were built from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed that up, and also updated the nuspec to:
- Explicitly list the content files (so a failure occurs if you forget to copy over the native library)
- Reuse the same LICENSE.TXT file from the
packages/libclang
directory, so we don't need 12 copies of it.
This adds some nuspecs for the libclang package to make it easier to create in the future. This also does some misc infrastructure upgrades to make it easier to consume the package once published.