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

Integrate zig package #6

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

hez2010
Copy link

@hez2010 hez2010 commented Nov 30, 2023

Closes #5

@MichalStrehovsky
Copy link
Owner

Thank you! Could you make the new logic conditional on some property so people who want to use Zig that they already have installed can do so (and the NuGet doesn't download)? Would you mind updating the README.md too?

@hez2010
Copy link
Author

hez2010 commented Dec 1, 2023

Done. PTAL.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Crosscompile.targets Outdated Show resolved Hide resolved
src/Crosscompile.targets Outdated Show resolved Hide resolved
src/PublishAotCross.targets Outdated Show resolved Hide resolved
@MichalStrehovsky
Copy link
Owner

I tried this out in a Windows Sandbox and it doesn't quite work (the package doesn't get downloaded and the property that should have path to the package is empty). I think it's because one cannot reference new NuGets from a .targets that is in a NuGet itself. NuGet restore doesn't see it and it kind of makes sense. NuGet restore is a different codepath from what the test project in this repo uses :/.

@hez2010
Copy link
Author

hez2010 commented Dec 6, 2023

I'm wondering how does the Microsoft.DotNet.ILCompiler package handle this correctly to download the corresponding runtime package?

@MichalStrehovsky
Copy link
Owner

There's some special logic for that in the sdk.

I think nuget has a way to express this package depends on something else in the package metadata. That's the normal way. Ilcompiler is definitely not normal in this sense, don't look at that

@alexrp
Copy link

alexrp commented Dec 11, 2023

AFAIK the (undocumented) runtime.json file in a NuGet package is the only way to express this notion. It's what e.g. LLVMSharp and ClangSharp do as well. This is for target RID, not host RID.

@MichalStrehovsky
Copy link
Owner

Adding a note of what I wrote on Discord: can we build this in a way that requires users to add Vezel.Zig.Toolsets.XXX manually? The targets can detect whether this nuget is present. If it is, use Zig from there. If it isn't, use zig from environment. If no zig is found, suggest to the user to either download zig manually and add to their path, or add a reference to the Vezel.Zig.Toolsets.XXX nuget.

@alexrp
Copy link

alexrp commented May 10, 2024

Adding a note of what I wrote on Discord: can we build this in a way that requires users to add Vezel.Zig.Toolsets.XXX manually?

It seems like that's the least painful way forward. Those packages will set ZigExePath, which PublishAotCross can check for and use.

I think we'd need some docs to point to that would explain that the recommended usage pattern for those packages is something like:

<Project Sdk="Microsoft.NET.Sdk">
    ...

    <ItemGroup>
       <PackageReference Include="Vezel.Zig.Toolsets.$(NETCoreSdkPortableRuntimeIdentifier)"
                         Version="0.12.0.*"
                         PrivateAssets="all" />
    </ItemGroup>

    ...
</Project>

That can probably just go in the README or something.

@VAllens
Copy link

VAllens commented Aug 29, 2024

Looks great, any progress on this PR?

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.

Consider using Zig toolset packages for a more convenient user experience
4 participants