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

Allow building net471 target without Windows #616

Merged
merged 1 commit into from Sep 19, 2021

Conversation

lostmsu
Copy link
Contributor

@lostmsu lostmsu commented Sep 11, 2021

No description provided.

Copy link
Collaborator

@MoFtZ MoFtZ left a comment

Choose a reason for hiding this comment

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

QUESTION: Microsoft.NETFramework.ReferenceAssemblies is an interesting Nuget package - first time I have heard of it. Should we be using the more specific Microsoft.NETFramework.ReferenceAssemblies.net471?

Also, if we plan on using this Nuget package, we should apply it to all the other locations in ILGPU too. e.g. unit test projects.

@lostmsu
Copy link
Contributor Author

lostmsu commented Sep 13, 2021

For the reference, official doc.

AFAIK Microsoft recommends using the "meta-package", however, I used specific version too without any issues.

Only the projects, that target .NET Framework need this. It just allows .NET SDK to build net4xx target on non-Windows OS and on Windows OS where SDK targeting pack is not installed.

I noticed now, that test projects actually have net471 too, but only for GitHub actions. I don't think this change is needed for GitHub actions, because unless you want to add Mono to your test matrix, there's no point in building net471 test target on non-Windows.

Actually, there's one advantage to adding this package to tests: building with .NET SDK would be possible without installing targeting pack workloads with Visual Studio. However, in Visual Studio itself it would not be enough unfortunately, until Microsoft fixes corresponding issue.

@lostmsu
Copy link
Contributor Author

lostmsu commented Sep 17, 2021

@MoFtZ do you want me to rebase this?

@MoFtZ
Copy link
Collaborator

MoFtZ commented Sep 17, 2021

This is unlikely to cause conflicts, so no need to rebase. Just wait for @m4rs-mt to review.

Copy link
Owner

@m4rs-mt m4rs-mt left a comment

Choose a reason for hiding this comment

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

Looks also good to me 👍 Thanks a lot for this simplification!

@m4rs-mt m4rs-mt merged commit fac8649 into m4rs-mt:master Sep 19, 2021
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.

None yet

3 participants