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

Enable SignFile on .NET Core/5+ #6509

Merged
merged 2 commits into from
Jun 2, 2021
Merged

Enable SignFile on .NET Core/5+ #6509

merged 2 commits into from
Jun 2, 2021

Conversation

Zastai
Copy link
Contributor

@Zastai Zastai commented Jun 2, 2021

Fixes #6098

Context

The SignFile task is referenced by the SDK for both .NET Framework and Core/5+, but not actually included in the non-framework assemblies.

Changes Made

Include SignFile.cs in the non-framework compilation.

Testing

A simple test, applying SignFile to one of the DLLs in artifacts using a self-signed certificate, succeeded.

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Might be useful to throw in a unit test? I don't think it's critical, and the change looks good to me. Thanks!

@Zastai
Copy link
Contributor Author

Zastai commented Jun 2, 2021

I considered it - but I'm not sure how I would set up a unit test that relies on entries in the certificate store (plus potentially a timestamp server).

If there is already a unit test for SignFile that only runs on .NET Framework, then yes, it should now be enabled on .NET Core/5+ too, but I don't think there is one.

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

I was hoping for tests too but I agree that there don't appear to be any. Alas. Thanks!

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jun 2, 2021
@Zastai
Copy link
Contributor Author

Zastai commented Jun 2, 2021

Incidentally, I noticed that the main branch produces MSBuild version 17 (i.e. VS2020).
What is the procedure for backports?
If this gets merged, will it automatically get included in the next .NET Core 3.1 and 5.0 SDKs?
If not, what do I need to do? Make separate PRs based on particular branches?

@Forgind
Copy link
Member

Forgind commented Jun 2, 2021

It will not. We have vs* (like vs16.11) branches that are for supporting earlier versions, so we'd have to merge into one of those branches. There's automation for bringing changes from vs* to main but not vice versa.

We decided that we can backport this to vs16.11 (so the 5.0 SDK), but we don't think this is high-priority enough to bring it back to 16.10 or before. Is there a reason you think it is?

As far as backporting it to vs16.11, you can just retarget this to vs16.11, but that would likely bring in extra commits we don't want, so please also clean that up afterwards.

@Forgind Forgind removed the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jun 2, 2021
@Zastai
Copy link
Contributor Author

Zastai commented Jun 2, 2021

I suppose the 5.0 SDK is fine, but given 3.1 is still in LTS for a good while, it seems to make sense to have it there too.

Would 16.11 mean "next 5.0 SDK gets it" or "next 5.0 feature band gets it"? If the latter, I might aim for 16.10 because that gets me the fix faster.

Given the triviality of the change, I'll look at just recreating the PR branch on my end and force-pushing, unless there's a reason not to do it that way.

Just let me know to base it on 16.10 or 16.11. Will convert to a draft for now.

@Zastai Zastai marked this pull request as draft June 2, 2021 18:16
@Forgind
Copy link
Member

Forgind commented Jun 2, 2021

LTS is more about whether we will provide fixes for critical bugs rather than whether we will enable new features, and this feels more like a feature to me than a bug fix. If it's in 16.11, that will go into the next release of the 5.0 SDK. Our team agreed that's best, so please rebase this on 16.11 after you've done the force push. Thank you!

@Zastai Zastai changed the base branch from main to vs16.11 June 2, 2021 19:02
@Zastai Zastai marked this pull request as ready for review June 2, 2021 19:03
@Zastai
Copy link
Contributor Author

Zastai commented Jun 2, 2021

I think that should do it.

@rainersigwald
Copy link
Member

Would 16.11 mean "next 5.0 SDK gets it" or "next 5.0 feature band gets it"?

MSBuild 16.11 will ship in .NET SDK 5.0.400.

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jun 2, 2021
@Forgind
Copy link
Member

Forgind commented Jun 2, 2021

@rainersigwald, something's up with those last two legs. They've been running for over 2 hours and haven't cancelled themselves, which also means I don't see error messages or logs. Do you have any suggestions on how to debug that?

@rainersigwald
Copy link
Member

I don't know what's up with the checks. I suspect it's because those PR jobs exist in main but not vs16.11. I'll bypass the policy to land this.

@rainersigwald rainersigwald merged commit 5e37cc9 into dotnet:vs16.11 Jun 2, 2021
@rainersigwald
Copy link
Member

Thanks @Zastai!

@Zastai Zastai deleted the issue-6098-core-signfile branch June 2, 2021 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SignFile task referenced, but not provided, in Microsoft.Build.Tasks.Core
3 participants