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 MSIX support #1522

Merged
merged 24 commits into from Jun 9, 2019

Conversation

Projects
None yet
3 participants
@onovotny
Copy link
Contributor

commented Jun 4, 2019

This PR is to add MSIX support. Along the way, it also adds Azure Pipeline support as a baseline.

Fixes #1450

To Do:

  • Needs a location for CI distribution
  • Needs code signing for CI package
  • Troubleshoot failing test on Pipelines / 2019 as per thread below
  • Store needs entry created
  • How do we want this all to work?

onovotny added some commits Jun 4, 2019

@onovotny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@christophwille I'm seeing test failures when built with VS 2019 on Pipelines, it's probably a simple fix but I'm not entirely sure where to look:

https://dev.azure.com/onovotny/GitBuilds/_build/results?buildId=1866&view=logs&jobId=3de91260-98a9-5dd9-3131-374192cb7ed7&taskId=860a41ab-aa3a-586b-99e3-a3f95437ff27&lineStart=421

Ring a bell?

62) Failed : Mono.Cecil.Tests.MethodTests.ReturnGenericInstanceWithMethodParameter
The assembly is built by a runtime newer than the currently loaded runtime, and cannot be loaded.1 Error(s) Verifying C:\Users\VssAdministrator\AppData\Local\Temp\cecil-drt\Generics.cs.dll
at NUnit.Framework.Assert.Fail(String message, Object[] args)
at Mono.Cecil.Tests.CompilationService.Verify(String name)
at Mono.Cecil.Tests.TestRunner.RoundTrip(String location, ReaderParameters reader_parameters, String folder)
at Mono.Cecil.Tests.TestRunner.RunTest()
at Mono.Cecil.Tests.BaseTestFixture.Run(TestCase testCase)
at Mono.Cecil.Tests.MethodTests.ReturnGenericInstanceWithMethodParameter()

onovotny added some commits Jun 4, 2019

@christophwille

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@siegfriedpammer @dgrunwald any ideas on those test failures? AppVeyor doesn't complain.

@siegfriedpammer

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Is peverify accessible in that environment?

@onovotny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@siegfriedpammer should be, though I've never used it myself

@onovotny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

One note about AppVeyor -- I believe the VS 2019 image also has VS 2017 on it, doesn't it? I ask because the RoundTripAssembly test looks for VS 2017 and fails if it's not present. Pipelines (and myself locally) do not have 2017 installed anymore. I had to update the MSBuild detection logic to work with 2019 -- so it might not work on appveyor either when using the 2019 msbuild.

@christophwille

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

According to https://www.appveyor.com/docs/windows-images-software/ (where the preview image is not listed explicitly) it does not look like that multiple VS versions are installed in one image.

But AppVeyor seems to be happy: https://ci.appveyor.com/project/icsharpcode/ilspy/builds/25043742

@onovotny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@christophwille seems odd then because that registry key doesn't exist for me on a clean 2019 install (machine never had 2017 on it): https://github.com/icsharpcode/ILSpy/blob/master/ICSharpCode.Decompiler.Tests/RoundtripAssembly.cs#L195

Also, the path to MSBuild is different in 2019. https://github.com/icsharpcode/ILSpy/blob/master/ICSharpCode.Decompiler.Tests/RoundtripAssembly.cs#L206

It's no longer in msbuild\15.0\bin, it's in msbuild\current\bin. So either the test isn't running or it's using 2017 on their agent. It can't possibly be using 2019 :)

@onovotny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

If it helps, this should repro locally as well with the updated MSBuild detection logic that ensures 2019 is used. Check out this PR branch, comment out the Ignore on line 42 (Cecil_net45()) and the test will fail.

@siegfriedpammer

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I cannot reproduce this locally using your branch with only VS 2019 installed.

onovotny added some commits Jun 5, 2019

@onovotny onovotny marked this pull request as ready for review Jun 5, 2019

@onovotny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

The tech part of this is pretty much done. It generates all of the packages/zip files and can replace AppVeyor if you choose. Pipelines is free for OSS projects. I have not yet done the release management part as that should be done in the Pipelines account we'll use. If you want to see what the build looks like, it's here for now: https://dev.azure.com/onovotny/GitBuilds/_build/results?buildId=1885

Do we use mine, is there one already?

We'll need a code signing cert for the CI version for sideloading. Does ICSharpCode have one already? If not, it's a very good reason to join the .NET Foundation. We provide EV code signing certs to member projects for free, which means you'll never have any smart screen issues on new releases again. We can follow that thread up separately.

"How we want this to work" would depend on where the CI/CD ultimately sits. If it's a project Pipeline, it's pretty easy to make this all seamless. I can set it up to create a GitHub release and push to the store upon approval of specific builds. CI would get pushed out continuously.

@christophwille

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

There is no pipelines account on our side (back in February I wanted to give it a try, but failed twice to link it and gave up) nor any code signing certs.

@onovotny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

How would you like to proceed? My recommendation is to join the .NET Foundation, I know we’d love to have you.

That provides many services, including a Pipelines project on our account, certs, and a CLA bot for ensuring contributions are okay. Is that something you’d be interested in? The Foundation doesn’t interfere with how you want to run your project, it’s realky there to help support you.

@christophwille

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

I cannot reproduce this locally using your branch with only VS 2019 installed.

I had a chat with Siegfried - he had VS2017 on the box, but uninstalled (so technically, he has only VS2019 on it, but who knows what the uninstall left behind)

@christophwille

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

In-private window to the rescue I was able to connect Azure Pipelines https://icsharpcode.visualstudio.com/icsharpcode-pipelines (empty cause it won't let me select the existing yaml on a PR branch and I have a feeling it needs more configuration than just that).

Now for my understanding: Store submission doesn't need a real cert. Only CI would if the dev cert isn't installed first (at least that was the sideloading story way back).

siegfriedpammer added some commits Jun 6, 2019

@siegfriedpammer

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

The unit testing problem should be fixed now: In icsharpcode/ILSpy-tests@28f74a2 I patched the DLL to include paths to newer versions of the SDKs.

@christophwille

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

A bit more color to the fix from Siegfried - we are using an older version of Mono.Cecil.Tests.dll (in the Tests repository) for the test. Its code reads like this

using System;
using System.IO;

private static string WinSdkTool(string tool)
{
	string[] array = new string[3]
	{
		"Microsoft SDKs\\Windows\\v8.1A\\bin\\NETFX 4.5.1 Tools",
		"Microsoft SDKs\\Windows\\v8.0A\\bin\\NETFX 4.0 Tools",
		"Microsoft SDKs\\Windows\\v7.0A\\Bin"
	};
	string[] array2 = array;
	foreach (string path in array2)
	{
		string path2 = (IntPtr.Size == 8) ? Environment.GetEnvironmentVariable("ProgramFiles(x86)") : Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles);
		string text = Path.Combine(Path.Combine(path2, path), tool + ".exe");
		if (File.Exists(text))
		{
			return text;
		}
	}
	return tool;
}

// Mono.Cecil.Tests.ShellService
public static ProcessOutput PEVerify(string source)
{
	return RunProcess(WinSdkTool("peverify"), "/nologo", Quote(source));
}

It does actually find peverify, but only really the fallback (3rd one), which is .NET 3.5 and errors out like this:

C:\cecil-drt>"C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\\Bin\PEVerify.exe" RetargetableExample.dll

Microsoft (R) .NET Framework PE Verifier.  Version  3.5.30729.1
Copyright (c) Microsoft Corporation.  All rights reserved.

The assembly is built by a runtime newer than the currently loaded runtime, and cannot be loaded.
1 Error(s) Verifying RetargetableExample.dll

Newer versions look like this https://github.com/jbevain/cecil/blob/master/Test/Mono.Cecil.Tests/CompilationService.cs#L356 but updating was not an option, so Siegfried patched the binary.

@onovotny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@christophwille thanks for the explanation, it would have taken me a long time to find that!

The store does not need a signed build, they sign it for you during the publish. The cert is needed for a sideloaded/CI package. The cert (particularly an EV cert), also helps with your "regular" exe release since it'll prevent SmartScreen warnings on new releases.

@christophwille

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

@onovotny Been thinking a bit about the options (and associated potential timelines) for the store builds. In the short term doing code signing isn't an option for us. Setting up a store account on the other hand is easy (for the "manual" releases).

Could we keep the CI builds on your end (as well as distribution)? If possible.

@onovotny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

I'm certainly happy to do CI builds, but am a bit reluctant about using my certificate for it. Can we get you into the .NET Foundation where we can issue you a certificate in the project's name (better yet, in ICSharpCode's name)?

@christophwille

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

I have created an issue on joining dotnetfdn #1525 - please add information on what would be necessary on our end (I presume you know that). Then we can have a discussion there and keep technical stuff here.

Code-wise, this PR is done. If we merge now, I could set up a pipeline with the yaml and get bits that are uploadable to the store (no CI with auto-update though). Would that be an acceptable short-term solution?

@onovotny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Getting it into the store would be amazing! I'm happy to setup a release management pipeline in the Pipelines instance that lets you upload to the store there with an approval button. There's a one time setup that you need as is documented here in the extension:
https://marketplace.visualstudio.com/items?itemName=MS-RDX-MRO.windows-store-publish

Once that's done, after the first submission, the rest can be done via the release process.

@christophwille

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

As much as I love AAD in a corporate setting (and as a dev), that is definitely overkill for our open source project. Especially because https://partner.microsoft.com/en-us/dashboard/registration/AccountInfo would be Individual for us ("Develop and sell apps, add-ins, and services as an individual, student, or unincorporated group")

This was referenced Jun 9, 2019

@christophwille

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

Created two issues to track the ToDos for MSIX CI drop location/certificate as well as store publishing.

@christophwille christophwille merged commit 76d5320 into icsharpcode:master Jun 9, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.