Generate shim with PackAsTool #2239

wli3 · 2018-05-11T22:07:26Z

Add PackageToolShimRuntimeIdentifiers property to the csproj. And the result nupkg will have shims which can be signed.

wli3 · 2018-05-11T22:28:05Z

src/Tasks/Common/Resources/Strings.resx

+  <data name="CannotFindApphostForRid" xml:space="preserve">
+    <value>Cannot find Apphost for {0}. {0} could be an invalid RuntimeIdentifier</value>


@mairaw could you help us review this error message while @KathleenDollard is in Europe?

This is the error message shown when (most likely) users used a wrong runtime identifier like win-x63. Or it is a rid that is not supported

My comments @wli3:

should Apphost really be one word?

RuntimeIdentifier -> runtime identifier (RID)

missing punctuation in the last sentence

should we add a fwlink to the RID topic for more info? https://docs.microsoft.com/dotnet/core/rid-catalog

Will, is it practical to search our error/warning messages for Apphost to see how we usually do it. I would like consistency on that. Apphost, AppHost, or application host all make sense to me, although app host not as much.

Same with RID vs RuntimeIdentifier. Do you have thoughts on the best place to keep notes that will help us be consistent on stuff like this?

A link to valid RIDs would be great here.

@mairaw @KathleenDollard

Apphost

according to https://docs.microsoft.com/en-us/dotnet/core/, it should be "app host". As in The 'dotnet' app host, which is used to launch .NET Core apps. It selects the runtime and hosts the runtime, provides an assembly loading policy and launches the app. The same host is also used to launch SDK tools in much the same way.

should we add a fwlink to the RID topic for more info?

created https://aka.ms/rid-catalog

is it practical to search our error/warning messages for Apphost to see how we usually do it. I would like consistency on that. Apphost, AppHost, or application host all make sense to me, although app host not as much.

should be "app host". That doc is kind of a "front page" of dotnet core

Same with RID vs RuntimeIdentifier. Do you have thoughts on the best place to keep notes that will help us be consistent on stuff like this?

"runtime identifier (RID)" should be the standard. As in docs.microsoft.com

so what about that

Cannot find app host for {0}. {0} could be an invalid runtime identifier (RID). For more information about RID https://aka.ms/rid-catalog.

@mairaw @KathleenDollard ?

I can change the wording on docs if it's preferred.
Only minor suggestion:
For more information about RID, see https://aka.ms/rid-catalog.

And can you add me as a co-owner of that aka.ms link?

Thanks, I prefer your version. And I added you to the aka.ms link

Fixed, changed to discussed

wli3 · 2018-05-11T22:29:17Z

@KathleenDollard or @richlander could you review the property name PackageToolShimRuntimeIdentifiers ?

it is used in csproj like

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

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.1</TargetFramework>
    <PackageToolShimRuntimeIdentifiers>win-x64;ubuntu-x64</PackageToolShimRuntimeIdentifiers>
    <PackAsTool>true</PackAsTool>

  </PropertyGroup>

</Project>

KathleenDollard · 2018-05-12T09:03:27Z

Regarding PackageToolShimRuntimeIdentifiers, I am not crazy about that name.

Since this is parallel to <RuntimeIdentifiers> I agree with your call not to abbreviate that. But I don't think need clarity with much else, so how about:

<ToolShimRuntimeIdentifiers>

Yes it is for the package, but that's true on <RuntimeIdentifiers> as well.

wli3 · 2018-05-14T05:21:46Z

@KathleenDollard

if I see ToolShimRuntimeIdentifiers in a csproj file. It is really hard to make the connection that it is for the result nupkg, not the project. while RuntimeIdentifier is fine since it directly relate to the project. What do you think? I want to convey "Run pack and these runtime identifiers shims will be generated"

Also the second question. Do you think it is a good idea to imply PackAsTool=true when above field is set?

KathleenDollard · 2018-05-14T06:47:57Z

OK. What about

<PackAsToolRuntimeIdentifiers>

wli3 · 2018-05-14T17:22:08Z

Talked to Kathleen in the morning. The current thinking is PackAsToolShimRuntimeIdentifiers

We don't want to imply PackAsTool = true. If people look at <PackAsToolShimRuntimeIdentifiers>win-x64;osx-x64</PackAsToolShimRuntimeIdentifiers>, they may think it only supports win and osx, while it supports all rid. So an explicit PackAsTool can help this situation.

As a result, if PackAsTool = false and PackAsToolShimRuntimeIdentifiers has something, we will error out.

wli3 · 2018-05-14T19:28:07Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets

    <NETSdkError Condition=" '$(_TargetFrameworkVersionWithoutV)' &lt; '2.1' "
                 ResourceName="DotnetToolDoesNotSupportTFMLowerThanNetcoreapp21" />

    <ItemGroup>
-        <_GeneratedFiles Include="$(PublishDepsFilePath)"/>


format change... sorry about that. However that's not too much and this happens several times.

nguerrera · 2018-05-15T18:15:26Z

I find PackAsToolShimRuntimeIdentifiers a little long, but it is clear, so 👍.

nguerrera · 2018-05-15T19:57:59Z

src/Tasks/Microsoft.NET.Build.Tasks/GenerateShims.cs

+        /// The RuntimeIdentifiers that shims will be generated for.
+        /// </summary>
+        [Required]
+        public ITaskItem[] PackageToolShimRuntimeIdentifiers { get; set; }


PackageToolShimRuntimeIdentifiers [](start = 27, length = 33)

I think we should rename this to match the property, or shorten it and decide there's enough context here to just "ShimRuntimeIdentifiers" or even "RuntimeIdentifiers". Right now, the unnecessary subtle difference between "PackAsToolShimRuntimeIdentifiers" and "PackageToolShimRuntimeIdentifiers" is confusing me.

Fixed, changed to ShimRuntimeIdentifiers

nguerrera · 2018-05-15T20:02:26Z

src/Tasks/Microsoft.NET.Build.Tasks/GenerateShims.cs

+        private static string NormalizeRelativePath(string relativePath)
+        {
+            return relativePath.Replace('/', Path.DirectorySeparatorChar);
+        }


This looks copied from ResolvePackageAssets before this and this

You should remove this and call ResolvePackageAssetPath on the resolver.

Fixed, removed all these and just call nugetpackageresolver

nguerrera · 2018-05-15T20:04:17Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets

+    <ItemGroup>
+      <_PackageToolShimRuntimeIdentifiers Include="$(PackageToolShimRuntimeIdentifiers)">
+
+      </_PackageToolShimRuntimeIdentifiers>


Nit: use self-closing tag.

<_PackageToolShimRuntimeIdentifiers Include="$(PackageToolShimRuntimeIdentifiers)" />

nguerrera · 2018-05-15T20:04:54Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets

+
+    <GenerateShims
+      DotNetAppHostExecutableNameWithoutExtension="$(_DotNetAppHostExecutableNameWithoutExtension)"
+      PackagedShimOutputDirectory="$(IntermediateOutputPath)/$(Configuration)/shims/$(targetframework)"


(targetframework [](start = 85, length = 16)

Nit: casing: $(TargetFramework)

nguerrera · 2018-05-15T20:08:54Z

src/Tests/Microsoft.NET.TestFramework/Commands/ShimCommand.cs

+            else
+            {
+                sdkCommandSpec.Environment.Add("DOTNET_ROOT(x86)", dotnetRoot);
+            }


Maybe we should do this for all tests here:

sdk/src/Tests/Microsoft.NET.TestFramework/ToolsetInfo.cs

Line 51 in f0ead56

public void AddTestEnvironmentVariables(SdkCommandSpec command)

fixed moved to AddTestEnvironmentVariables

nguerrera · 2018-05-15T20:10:26Z

src/Tasks/Microsoft.NET.Build.Tasks/sdk/Sdk.targets

@@ -49,6 +49,7 @@ Copyright (c) .NET Foundation. All rights reserved.
  <PropertyGroup Condition="'$(NuGetBuildTasksPackTargets)' == '' AND '$(ImportNuGetBuildTasksPackTargetsFromSdk)' != 'false'">
    <IncludeBuildOutput Condition=" '$(PackAsTool)' == 'true' ">false</IncludeBuildOutput>
    <PackageType Condition=" '$(PackAsTool)' == 'true' ">DotnetTool</PackageType>
+    <RuntimeIdentifiers Condition=" '$(PackAsTool)' == 'true' ">$(RuntimeIdentifiers);$(PackageToolShimRuntimeIdentifiers)</RuntimeIdentifiers>


It looks wrong that this is in this property group. Why does the condition apply to it?

It need to be set early enough before nuget is loaded. Only when PackAsTool=true aka use dotnet pack to create global tools this is useful. Also I want to add error when PackAsTool!=true and PackageToolShimRuntimeIdentifiers is set. According to what I talked to Kathleen

Note I did not move this logic due to the reason above. It is out of date due to rename change

nguerrera · 2018-05-15T20:12:56Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets

    </ItemGroup>
  </Target>

+


nit: extra blank line

nguerrera · 2018-05-15T20:13:48Z

src/Tasks/Microsoft.NET.Build.Tasks/GenerateShims.cs

+        public string ToolEntryPoint { get; set; }
+
+        /// <summary>
+        /// The output Directroy path of generated shims.


typo: directory. (Also, nit, I wouldn't capitalize it.)

nguerrera · 2018-05-15T20:16:20Z

src/Tasks/Microsoft.NET.Build.Tasks/GenerateShims.cs

+                    Path.Combine(packagedShimOutputDirectoryAndRid,
+                        $"{ToolCommandName}{Path.GetExtension(resolvedApphostAssetPath)}");
+                var appBinaryFilePath =
+                    $".store/{PackageId.ToLowerInvariant()}/{PackageVersion}/{PackageId.ToLowerInvariant()}/{PackageVersion}/tools/{targetFramework.GetShortFolderName()}/any/{ToolEntryPoint}";


Strange to see this next to other Path.Combine calls. I think Path.Combine with each portion of path as an arg on its own line would be better.

This is the embedded string. If we use Path.Combine, the embedded file will be different according to which machine builds it. \ build on win, / build on Linux. Although technically apphost is fine with that.

I will add comments and move it away from other Path.Combine

// This is the embedded string. We should normalize it on forward slash, so the result apphost won't be different according to
// build machine.

Will this look better?

// This is the embedded string. We should normalize it on forward slash, so the file won't be different according to // build machine. var appBinaryFilePath = string.Join("/", new[] { ".store", PackageId.ToLowerInvariant(), PackageVersion, PackageId.ToLowerInvariant(), PackageVersion, "tools", targetFramework.GetShortFolderName(), "any", ToolEntryPoint});

changed as above

nguerrera · 2018-05-15T20:17:20Z

src/Tasks/Microsoft.NET.Build.Tasks/GenerateShims.cs

+                var packagedShimOutputDirectoryAndRid = Path.Combine(PackagedShimOutputDirectory, runtimeIdentifier);
+                var appHostDestinationFilePath =
+                    Path.Combine(packagedShimOutputDirectoryAndRid,
+                        $"{ToolCommandName}{Path.GetExtension(resolvedApphostAssetPath)}");


nit: I find ToolCommandName + Path.GetExtension(resolvedApphostAssetPath) easier to read. It's also faster than string.Format. but perf isn't really my concern here.

fixed, changed as suggested

nguerrera · 2018-05-15T20:20:44Z

src/Tasks/Microsoft.NET.Build.Tasks/GenerateShims.cs

+                if (File.Exists(appHostDestinationFilePath))
+                {
+                    File.Delete(appHostDestinationFilePath);
+                }


This is because for other contexts, Apphost.Create assumes if host is already written, it doesn't need to be regenerated?

I think it would be better to have a bool overwriteExisting parameter on AppHost.Create.

Just so I understand, is the issue that the args to Create can change without the path changing, hence we need to recreate it every time?

Correct. I had a bug during my test. So I have to enforce recreate all the time. I will add bool overwriteExisting param to AppHost.Create

wli3 · 2018-05-15T20:22:45Z

@mairaw @KathleenDollard
Sorry I forget this error message. When PackAsTool is not true and PackAsToolShimRuntimeIdentifiers is not empty, we decided to error it out. Since PackAsToolShimRuntimeIdentifiers requires PackAsTool=true, it is a configuration for PackAsTool. Here is my draft.

"PackAsToolShimRuntimeIdentifiers" is a property to configure tool package. It requires property "PackAsTool" being set to "true".

nguerrera · 2018-05-15T20:24:33Z

src/Tasks/Microsoft.NET.Build.Tasks/GenerateShims.cs

+        /// The output Directroy path of generated shims.
+        /// </summary>
+        [Required]
+        public string PackagedShimOutputDirectory { get; set; }


(Sorry in advance, I think this came up in the meeting, but I forget the answer.)

What's the story for signing the shims put in this directory? Are we relying on folks to repack nupkgs?

PackagedShimOutputDirectory currently set to temporary folder under BaseIntermediateOutputPath. The producer (code gen in this case) need to unpack, sign and repack.

Their ask is to generate shim during publish instead of pack. We have not promised any further feature yet.

I maybe able to just move this task execution from pack to publish. However, there are some package context needed like packageid, packageversion which is used in part of the embedded path. Maybe that's fine due to they are properties. But we do need some design on that interface. #Resolved

Like I said on the overall review comment, we can talk about this separately and (maybe) do it as a follow-up. Can you file an issue to track it? #Resolved

created https://github.com/dotnet/cli/issues/9288

In reply to: 188433576 [](ancestors = 188433576)

nguerrera

Looks good overall. Lots of comments but they're actually pretty minor.

The signing thing is something I think might end up needing iteration, but I am fine leaving that discussion for a future change.

peterhuene · 2018-05-16T00:59:46Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

@@ -69,7 +69,8 @@ Copyright (c) .NET Foundation. All rights reserved.
    <_NativeExecutableExtension Condition="'$(_NativeExecutableExtension)' == '' and $(RuntimeIdentifier.StartsWith('win'))">.exe</_NativeExecutableExtension>

    <_DotNetHostExecutableName>dotnet$(_NativeExecutableExtension)</_DotNetHostExecutableName>
-    <_DotNetAppHostExecutableName>apphost$(_NativeExecutableExtension)</_DotNetAppHostExecutableName>
+    <_DotNetAppHostExecutableNameWithoutExtension>apphost</_DotNetAppHostExecutableNameWithoutExtension>


I'm trying to understand this change. It seems the only place we use _DotNetAppHostExecutableNameWithoutExtension is where there's a check to append .exe if on Windows anyways (the same check that's done to set _NativeExecutableExtension above).

Is there a reason we couldn't use _DotNetAppHostExecutableName for GenerateShims?

Is this just so that users can't override the extension in the task?

This change it is to get a clean string "apphost" and it is only in one place. The problem I cannot use "_DotNetAppHostExecutableName" is even on Windows, GenerateShims can create Linux/mac apphost. But at that point, it is apphost.exe passed in.

Oh, I see. It's using a different RID list to check against for adding the extension. Makes sense 👍.

wli3 · 2018-05-16T01:52:52Z

Actually... PackAsToolShimRuntimeIdentifiers might have the same problem of IsPackable. PackAsToolShimRuntimeIdentifiers could be set in directory.build.prop since that is all the rid this repo cares about. But only few project will have PackAsTool enabled, most of them might be libraries. In this case, we will error out on all libraries projects. And there is no easy way to "unset" that. Maybe we should just do nothing when PackAsToolShimRuntimeIdentifiers is set but PackAsTool is not set.

So no additional change on existing code.

@KathleenDollard @nguerrera @peterhuene

dsplaisted · 2018-05-16T02:11:16Z

Actually... PackAsToolShimRuntimeIdentifiers might have the same problem of IsPackable. PackAsToolShimRuntimeIdentifiers could be set in directory.build.prop since that is all the rid this repo cares about. But only few project will have PackAsTool enabled, most of them might be libraries. In this case, we will error out on all libraries projects. And there is no easy way to "unset" that. Maybe we should just do nothing when PackAsToolShimRuntimeIdentifiers is set but PackAsTool is not set.

Great point, 👍 to doing nothing in this case.

KathleenDollard · 2018-05-16T06:27:46Z

OK, Do nothing when it isn't sensible

nguerrera · 2018-05-16T18:17:55Z

src/Tasks/Microsoft.NET.Build.Tasks/sdk/Sdk.targets

@@ -49,6 +49,7 @@ Copyright (c) .NET Foundation. All rights reserved.
  <PropertyGroup Condition="'$(NuGetBuildTasksPackTargets)' == '' AND '$(ImportNuGetBuildTasksPackTargetsFromSdk)' != 'false'">
    <IncludeBuildOutput Condition=" '$(PackAsTool)' == 'true' ">false</IncludeBuildOutput>
    <PackageType Condition=" '$(PackAsTool)' == 'true' ">DotnetTool</PackageType>
+    <RuntimeIdentifiers Condition=" '$(PackAsTool)' == 'true' ">$(RuntimeIdentifiers);$(PackAsToolShimRuntimeIdentifiers)</RuntimeIdentifiers>


I believe this and the 2 options above it are out of place in the property group. I believe the intention here is that there is a NuGet package for the build tasks that can override the pack targets that come from the SDK. As is, if that is used, then it will break packing tools. I think these should be set unconditionally.

cc @rohit21agrawal

(By "unconditionally", I really mean conditioned only on PackAsTool == true.)

You mean like that?

 <PropertyGroup Condition="'$(NuGetBuildTasksPackTargets)' == '' AND '$(ImportNuGetBuildTasksPackTargetsFromSdk)' != 'false'"> <NuGetBuildTasksPackTargets Condition="'$(IsCrossTargetingBuild)' == 'true'">$(MSBuildThisFileDirectory)..\..\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets</NuGetBuildTasksPackTargets> <NuGetBuildTasksPackTargets Condition="'$(IsCrossTargetingBuild)' != 'true'">$(MSBuildThisFileDirectory)..\..\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets</NuGetBuildTasksPackTargets> <ImportNuGetBuildTasksPackTargetsFromSdk>true</ImportNuGetBuildTasksPackTargetsFromSdk> </PropertyGroup> <PropertyGroup> <IncludeBuildOutput Condition=" '$(PackAsTool)' == 'true' ">false</IncludeBuildOutput> <PackageType Condition=" '$(PackAsTool)' == 'true' ">DotnetTool</PackageType> <RuntimeIdentifiers Condition=" '$(PackAsTool)' == 'true' ">$(RuntimeIdentifiers);$(PackAsToolShimRuntimeIdentifiers)</RuntimeIdentifiers> </PropertyGroup>

I read this condition as"if there is no override pack target use Pack target from nuget's pack", which is most of the case. PackAsTool rely on nuget's Pack target, so it needs to be set only when use nuget's pack version.

However, if we think TfmSpecificPackageFile is a public interface that custom pack target should implement, We can remove the condition on ImportNuGetBuildTasksPackTargetsFromSdk

The whole point of that condition is to allow you to get nuget pack targets out of band from SDK. See #888.

Suppose the user wants to try a version with a bug fix over what's in the SDK. This should not prevent them from packing a tool with that configuration.

Also, this file, sdk/sdk.targets (main entry point) is also the wrong place to put code to handle specific scenarios. This should be closer to where PackAsTool is implemented.

Exactly as what you said. If I move these props to toolpack, it will fail on muti targeting. Microsoft.NET.Sdk.Common.targets is imported by both. So it is the right place.

wli3 · 2018-05-16T22:16:12Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Common.targets

@@ -43,6 +43,12 @@ Copyright (c) .NET Foundation. All rights reserved.
    <DotnetCliToolTargetFramework>netcoreapp$(BundledNETCoreAppTargetFrameworkVersion)</DotnetCliToolTargetFramework>
  </PropertyGroup>

+  <PropertyGroup>


@nguerrera so I moved them here

wli3 · 2018-05-16T22:16:58Z

I think all issues are resolved by now

wli3 requested review from nguerrera and dsplaisted May 11, 2018 22:23

wli3 changed the title ~~WIP Generate shim with PackAsTool~~ Generate shim with PackAsTool May 11, 2018

wli3 requested a review from a team May 11, 2018 22:24

wli3 commented May 11, 2018

View reviewed changes

wli3 commented May 14, 2018

View reviewed changes

wli3 force-pushed the generate-shim-packastool-05 branch from b3eb1fb to f5714e1 Compare May 14, 2018 20:45

nguerrera reviewed May 15, 2018

View reviewed changes

wli3 requested a review from a team May 15, 2018 19:59

nguerrera reviewed May 15, 2018

View reviewed changes

peterhuene reviewed May 16, 2018

View reviewed changes

William Li added 4 commits May 15, 2018 19:01

Add generate shim when use PackAsTool

749ddd2

loc

2f2e2a6

Use osx for test

2a1651b

Address code review

a75c716

wli3 force-pushed the generate-shim-packastool-05 branch from 4f50eb2 to a75c716 Compare May 16, 2018 02:01

Add missing env setting

a8ca747

nguerrera reviewed May 16, 2018

View reviewed changes

wli3 added this to the 2.1.4xx milestone May 16, 2018

Move props out of Sdk.targets

47de4cc

wli3 commented May 16, 2018

View reviewed changes

nguerrera approved these changes May 17, 2018

View reviewed changes

wli3 requested a review from livarcocc May 17, 2018 00:34

livarcocc added the Approved to merge label May 17, 2018

wli3 merged commit 36a8d1f into dotnet:release/2.1.4xx May 17, 2018

wli3 deleted the generate-shim-packastool-05 branch May 17, 2018 17:16

This was referenced Aug 10, 2018

EmbedAppNameInHost fails when calling build twice #2466

Closed

Fix cannot publish self contain twice #2467

Merged

		<data name="CannotFindApphostForRid" xml:space="preserve">
		<value>Cannot find Apphost for {0}. {0} could be an invalid RuntimeIdentifier</value>

Generate shim with PackAsTool #2239

Generate shim with PackAsTool #2239

Conversation

wli3 commented May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

so what about that

Choose a reason for hiding this comment

mairaw May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 commented May 11, 2018 • edited Loading

KathleenDollard commented May 12, 2018

wli3 commented May 14, 2018 • edited Loading

KathleenDollard commented May 14, 2018 • edited by wli3 Loading

wli3 commented May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 May 16, 2018 • edited Loading

Choose a reason for hiding this comment

wli3 May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 commented May 15, 2018 • edited Loading

Choose a reason for hiding this comment

wli3 May 15, 2018 • edited Loading

Choose a reason for hiding this comment

nguerrera May 15, 2018 • edited by wli3 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera left a comment

Choose a reason for hiding this comment

peterhuene May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhuene May 16, 2018 • edited Loading

Choose a reason for hiding this comment

wli3 commented May 16, 2018 • edited Loading

dsplaisted commented May 16, 2018

KathleenDollard commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 commented May 16, 2018

wli3 commented May 11, 2018 •

edited

Loading

mairaw May 15, 2018 •

edited

Loading

wli3 commented May 11, 2018 •

edited

Loading

wli3 commented May 14, 2018 •

edited

Loading

KathleenDollard commented May 14, 2018 •

edited by wli3

Loading

wli3 commented May 14, 2018 •

edited

Loading

wli3 May 16, 2018 •

edited

Loading

wli3 May 16, 2018 •

edited

Loading

nguerrera May 15, 2018 •

edited

Loading

wli3 commented May 15, 2018 •

edited

Loading

wli3 May 15, 2018 •

edited

Loading

nguerrera May 15, 2018 •

edited by wli3

Loading

peterhuene May 16, 2018 •

edited

Loading

peterhuene May 16, 2018 •

edited

Loading

wli3 commented May 16, 2018 •

edited

Loading

wli3 May 16, 2018 •

edited

Loading