Make it core-clr compatible and unix-friendly #329

vors · 2017-12-22T04:43:28Z

Fix appveyor and build.ps1
Fix all tests to be line-end agnostic
Replace powershell aliases by full names for cross-plat compatability
Remove old .nuget folder
Switch build.psd1 to dotnet cli
Make C# part CoreCLR-friendly
Update csproj files
update docs
add unix CI build

Fix #205

vors · 2017-12-22T04:44:05Z

@BernieWhite @it-praktyk can you take a look?

BernieWhite

@vors Nice work.

BernieWhite · 2017-12-22T12:32:53Z

src/Markdown.MAML/Markdown.MAML.csproj

-    <AssemblyName>Markdown.MAML</AssemblyName>
-    <TargetFrameworkVersion>v4.5</TargetFrameworkVersion>
-    <FileAlignment>512</FileAlignment>
+    <TargetFramework>netstandard1.6</TargetFramework>


Shouldn't we be targeting both net452 and netstandard1.6? We still need this to work on PowerShell 5.1 right?

It seems to work as is: AppVeyor CI runs full PowerShell 5.1

But I would not claim that I understand dotnet taxonomy. If it breaks on older versions of PS, we can revisit this.

Fairly sure this is because the framework is installed. I'll confirm on a fresh machine.

Looks like you are right, got that on a fresh server 2016 machine. Good catch!

Import-Module : Could not load file or assembly 'System.Runtime, Version=4.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified. At line:1 char:1 + Import-Module C:\Users\localadmin\Downloads\platyPS-0.7.6.1064\Markdo ... + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + CategoryInfo : NotSpecified: (:) [Import-Module], FileNotFoundException + FullyQualifiedErrorId : System.IO.FileNotFoundException,Microsoft.PowerShell.Commands.ImportModuleCommand

So is there a way to build 1 assembly that will work on ps 5.1 and 6.0?
Or two assemblies hack is the only way?

cc @daxian-dbw

I guess one way would be to build only net451 version and ship it (because it works just fine on pwsh for some reason), and for local development on non-windows platform build netstandart1.6
but that doesn't feel too good

Two assemblies for sure, can't see that as a hack though. It is how every other cross plat module is built afaik

well, you can use the currently published modules from the gallery (built for 4.5) just fine on pwsh. At least I haven't seen problems so far and problems are usually showing up very early when it cannot resolve System.Runtime and friends. Also a bunch of dlls, like makdig are working ok on both, when consumer thru import-module (it's also built for full). So we can still build for full and publish it as the one package.
It just that you cannot do that from a non-windows platform because full doesn't exist there.

BernieWhite · 2017-12-22T12:34:09Z

src/Markdown.MAML/Renderer/Markdownv2Renderer.cs


        public int MaxSyntaxWidth { get; private set; }

+        private const string NewLine = "\r\n";


Nice. We should probably do this generically. There is lot of separate references to \r\n

Yeah, I just didn't want to introduce dependencies between classes to reference 2 well-known chars and creating a common helper seems like an overkill too.

BernieWhite · 2017-12-22T12:38:26Z

src/Markdown.MAML/Renderer/RenderCleaner.cs

 namespace Markdown.MAML.Renderer
 {
-    static class RenderCleaner
+    static public class RenderCleaner


What is the reason for making it public? We should minimise exposure to classes that don't need to be called outside the assembly. As we add extensibility we want to minimise people taking a dependency on a class that is for internal use.

Agree. I used line normalization from this class in tests. What would be a better way?

BernieWhite · 2017-12-22T12:40:00Z

src/platyPS/platyPS.psd1


 # Assemblies that must be loaded prior to importing this module
-RequiredAssemblies = @('Markdown.MAML.dll','YamlDotNet.dll')
+RequiredAssemblies = @('Markdown.MAML.dll','YamlDotNet.NetStandard.dll')


Same comment as above. Needs to run on PS 5.1 unless we are explicitly dropping support for it.

AFAIK net451 implements NetStandard and 5.1 compatible. CI run proves that.

BernieWhite · 2017-12-22T12:41:48Z

src/platyPS/platyPS.psm1

            elseif (Test-Path -PathType Container $_)
            {
-                $MarkdownFiles += Get-ChildItem $_ -Filter $filter | WHERE {$_.BaseName -notlike $aboutFilePrefixPattern}
+                $MarkdownFiles += Get-ChildItem $_ -Filter $filter | Where-Object {$_.BaseName -notlike $aboutFilePrefixPattern}


Nice. Completely agree with expanding out aliases. Make it easier to understand for more people.

VS Code makes it no-brainer with automatic alias expansion refactoring.
I seen a bunch of folders like -ErrorAction in repo root, but hopefully fixed all mkdir calls.

… YamlDotNet

vors · 2017-12-24T01:33:10Z

Sorry, rebased on master and then realized that it will make the review harder, apologies for that.
@BernieWhite thank you so much for catching the TargetFramework problem! It was very close to breaking everybody on a new release. So glad you took a look. 👍

I employed the same technique as @lzybkr in PSReadLine: build for net4xx which is core-clr compatible in the pwsh context, because of netstandart2.0 (or not sure really why).
So currently we still need to publish only appveyor produced builds, they will work with the same dlls on all platforms.

But for a local development, build.ps1 will now use netstandart1.6 for a local build. Also, Travis CI will ensure there are no regressions in this case.
Overall that seems like a nicer alternative then shipping multiply dlls. The only downside is really inability to produce a package from a non-windows machine.

BernieWhite reviewed Dec 22, 2017

View reviewed changes

vors added 16 commits December 23, 2017 17:24

Update csproj files

46c27b5

Make C# part CoreCLR-friendly

7350ff6

xUnit tests pass on mac OS

6d4c73a

Switch build.psd1 to dotnet cli

aa2fd68

Remove old .nuget folder

3737879

Replace powershell aliases by full names for cross-plat compatability

6da0e7d

Fix FullLoop.Tests.ps1

6941064

Fix PlatyPs.Tests.ps1

6f96190

Fix appveyor and build.ps1

4991e89

Explicitly query IsLinux and IsMacOS in tests

5bdf4aa

Update build docs

07b1d44

Fix casing in tests and build.ps1

d74d2b4

don't overwrite IsLinux and IsOsMac

6cb3cf7

add .travis.yml

d1cbe9f

Silent Write-Progress on coreclr

65a2729

Make platyPS target net451 on windows and switch back to the official…

0b70fa5

… YamlDotNet

vors force-pushed the unix3 branch from 7c76727 to 0b70fa5 Compare December 24, 2017 01:25

vors merged commit 6746f01 into master Dec 24, 2017

vors deleted the unix3 branch December 24, 2017 02:09


		public int MaxSyntaxWidth { get; private set; }

		private const string NewLine = "\r\n";

Make it core-clr compatible and unix-friendly #329

Make it core-clr compatible and unix-friendly #329

Uh oh!

Conversation

vors commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vors commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BernieWhite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vors Dec 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vors Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vors commented Dec 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vors commented Dec 22, 2017 •

edited

Loading

vors commented Dec 22, 2017 •

edited

Loading

vors Dec 23, 2017 •

edited

Loading

vors Dec 22, 2017 •

edited

Loading