external/dir.props: remove Portable condition #21032

tmds · 2017-06-14T04:24:07Z

This PR checks what platforms fail when removing the PortableBuild condition in external/dir.props

ericstj · 2017-06-15T16:21:53Z

You're going to provide a much more detailed explanation about why you think you need to do this.

Explain what problem you are trying to solve, how this solves it, and what alternatives you considered.

We've spent a lot of time trying to simplify the number of RIDs floating around and how they are calculated, which take precedence, etc. This appears to be a step backwards.

ericstj · 2017-06-15T16:23:40Z

config.json

      "values": ["True", "False"],
      "defaultValue": false
    },
    "RuntimeOS": {


This is meant to represent what we're running on.

ericstj · 2017-06-15T16:23:51Z

config.json

+      "values": ["win7", "osx.10.10", "ubuntu.14.04", "ubuntu.16.04", "etc-other-rid-based-os-names"],
+      "defaultValue": ""
+    },
+    "TargetRid": {


This already exists today and is PackageRID.

tmds · 2017-06-15T16:28:37Z

@ericstj sure. Sorry for the lack of context. This is part of https://github.com/dotnet/corefx/issues/20731
The use case is: bootstrapping a build by a package maintainer.
I started this PR to get a better understanding of how things fit together. I'm sure things can be improved a lot.
The PR as it is currently splits the RuntimeOS in 3 properties:

HostRID: can be set to override the rid of the build system, e.g. tizen build needs this since there is no tizen x64 ilasm
RuntimeRid: used to select the runtime
TargetRid: this allows to name the build output

ericstj · 2017-06-15T16:48:00Z

Why couldn't you use the existing properties?

HostRID: can be set to override the rid of the build system, e.g. tizen build needs this since there is no tizen x64 ilasm

Don't plumb a new global property to deal with this. It looks to me like you're trying to build for a new RID: tizen-x64. The problem is that RID is missing from https://github.com/dotnet/corefx/blob/master/pkg/Microsoft.NETCore.Platforms/runtime.json#L366-L374. If it were present, then tizen would fallback to linux-x64 and you'd get that by default (after the package was published, and consumed by these projects).

RuntimeRid: used to select the runtime

This already exists, it's RuntimeOS. Architecture is passed separately. Why doesn't that work for you?

TargetRid: this allows to name the build output

This already exists, it's PackageRID.

Thinking about the whole scenario (building on a new RID), suppose you just specified RuntimeOS to some existing RID that was supported in order to bootstrap off of some existing binaries for toolchain, and specified PackageRID as your new RID?

tmds · 2017-06-16T04:21:37Z

@ericstj
I'm part of the team that maintains the .NET Core package for RHEL and part of the .NET SIG that maintains the package for Fedora.

So far, my experience regarding building .NET Core and RIDs is that:

all dotnet repos contain a case just below reading /etc/os-release that says: if this is rhel.7.x, then strip the minor.
corefx runtime.json contains fedora.26 to be sure we may be capable of building .NET Core on the next version of Fedora.

When bootstrapping:

If you try and do a non-portable build on an unsupported (i.e. not part of Tools' runtime.json) machine, it will fail.
First it will fail to pull external/ilasm, which can be fixed by RuntimeOS. And then it will fail to fetch external/runtime
because it is using the ilasm RID to fetch the runtime. This is what lead to splitting into HostRid and RuntimeRid.

To do a decent build, one would actually want to do two builds. The first build would be based on a 3rd party cli (e.g. Microsoft linux-x64 build).
This would generate the stage1. Then a second build would use the stage1 build the final build. This ensures that the package isn't built using 3rd party binaries.
This lead me to the TargetRid property.
e.g. the two stages on a linux-x64 compatible unknown banana.28-x64 linux would be:

build coreclr firstbuild
corefx: HostRID=linux-x64 RuntimeRID=firstbuild TargetRID=firstbuild

build coreclr banana.28 using firstbuild
corefx: HostRID=firstbuild RuntimeRID=banana.28 TargetRID=banana.28-x64

(Other than that, the TargetRid is usefull to handle the cases like the rhel renaming.)

Not included in this PR is a build task which updates runtime.json with the TargetRID if it is not yet present.
There is such a task as part of the source build project.

I should have shared this before, but I didn't expect you to be added as a code reviewer. I was using this PR to improve my understanding and experiment using the CI setup (since I only build for Linux locally).

I hope this properly explains the use-case. If you want, I can close the PR and update the issue (https://github.com/dotnet/corefx/issues/20731) with this text or we can continue the discussion here.

ericstj · 2017-06-16T15:50:22Z

/cc @ellismg and @chcosta who are working on the vertical build scenario.

First it will fail to pull external/ilasm, which can be fixed by RuntimeOS. And then it will fail to fetch external/runtime
because it is using the ilasm RID to fetch the runtime. This is what lead to splitting into HostRid and RuntimeRid.

You should be able to use a single RuntimeOS to select both a working ILASM and runtime. Please be specific in describing a case where this doesn't work. If that is so, fix the bug causing that case. Don't plumb more workarounds into the infrastructure.

As far as I can tell TargetRID seems functionally equivalent to PackageRID. Let me know if I am missing something here. Please be specific in describing why using PackageRID doesn't work. Likely its better to just fix bugs there than add new concepts...

It also sounds like you're trying to encode an intermediate phase into the runtimeID. Please don't do that. You can just as well manage that through paths/versioning which is consistent with how we normally flow bits across the repositories.

tmds · 2017-06-16T17:47:09Z

I made this PR to improve my understanding of the code and experiment. Some of these things I am trying will be possible by using (or extending) existing properties. This wasn't meant as a final PR.

You should be able to use a single RuntimeOS to select both a working ILASM and runtime. Please be specific in describing a case where this doesn't work. If that is so, fix the bug causing that case. Don't plumb more workarounds into the infrastructure.

This is when the rid of the host os is not in the Tools runtime.json. This is the case for every 'new' OS. It's similar to why Tizen has this line in ilasm.depproj: https://github.com/dotnet/corefx/blob/master/external/ilasm/ilasm.depproj#L8.

As far as I can tell TargetRID seems functionally equivalent to PackageRID. Let me know if I am missing something here. Please be specific in describing why using PackageRID doesn't work. Likely its better to just fix bugs there than add new concepts...

This may be a perfect match.

It also sounds like you're trying to encode an intermediate phase into the runtimeID. Please don't do that. You can just as well manage that through paths/versioning which is consistent with how we normally flow bits across the repositories.

Agree.

ericstj · 2017-06-16T22:49:47Z

I made this PR to improve my understanding of the code and experiment. Some of these things I am trying will be possible by using (or extending) existing properties. This wasn't meant as a final PR.

Understood.

The Tizen case seems like a mistake. Looking at #18725 it looks like @jyoungyun was specifying RuntimeOS when TargetRID would have been more appropriate. This is exactly why I don't want this stuff to get more complicated, its already complicated enough 😃

I would expect bootstrapping to look like:
build -- /p:RuntimeOS=<someExistingOS>;TargetRID=<newRID>

For example here would be "linux" or "ubuntu" in order to use all the runtime assets from that existing OS. Suppose you were trying to use some brand new RID here that you just built in CoreClr, it'd be a bug in ILAsm package if you built for that RID and the built package out of CoreCLR didn't support it.

TargetRID is the new RID you're building for.

jyoungyun · 2017-06-19T02:41:53Z

@ericstj @tmds The reason why I added #21032 is that Tizen has not published x64 packages yet according to our policy. So, I use ubuntu packages as fallback source. If we already have x64 packages, we do not need to add that line.

So, I think in Tizen case, it seems more appropriate to use RuntimeOS instead of targetRID.

If you have any other comments, please tell me. :)

ericstj · 2017-06-19T16:26:46Z

@jyoungyun, if you use RuntimeOS=ubuntu.14.04;TargetRID=tizen.4.0.0-armel wouldn't that work without a special case in the ILAsm project?

tmds · 2017-06-23T08:36:33Z

@ericstj I've adjusted the PR based on the discussion.

It is adding one new config option HostOS. This can be used when the cli under Tools does not know RuntimeOS (e.g. brand new linux flavor). On your open question to @jyoungyun on this: I think RuntimeOS=ubuntu.14.04;TargetRID=tizen.4.0.0-armel does not work for because that would restore ubuntu.14.04-armel in runtime.depproj where this must be tizen.4.0.0-armel.
The PR also refactors the use of RIDs: it centralizes the logic in dir.props and refers to HostRid and TargetRid from other places.
In dir.props I tried to calculate the portable RID based on OSGroup but this fails in CI, I don't understand why (see commended out XML).

ericstj · 2017-06-23T16:30:09Z

dir.props

+    <_portableRID Condition="'$(_runtimeOSFamily)' == 'win'">win</_portableRID>
+    <_portableRID Condition="'$(_runtimeOSFamily)' == 'osx'">osx</_portableRID>
+<!--
+    Why doesn't this work?


OSGroup is specific to the project configuration. For most projects which don't cross-compile by OSGroup this will be "AnyOS". I believe your change may break the cross-targeting we do in the runtime.depproj. In that case we try to restore both a Windows and a Unix version of the runtime separately, so that projects that build for Unix and use ReferenceFromRuntime get the Unix System.Private.CoreLib and those building for Windows get the Win SPC.

This is the "AllConfigurations" build concept. We need to maintain that build, but it doesn't need to support PackageRID. PackageRID concept clashes with "AllConfigurations" concept.

ericstj · 2017-06-23T16:31:15Z

dir.props

+    <HostRid>$(HostOS)-x64</HostRid>
+
+    <_runtimeOSVersionIndex>$(RuntimeOS.IndexOfAny(".-0123456789"))</_runtimeOSVersionIndex>
+    <_runtimeOSFamily Condition="'$(_runtimeOSVersionIndex)' != '-1'">$(RuntimeOS.SubString(0, $(_runtimeOSVersionIndex)))</_runtimeOSFamily>


We should make this RuntimeOSFamily if we intend to use it from multiple targets/project files.

ericstj · 2017-06-23T16:32:17Z

config.json

      "defaultValue": ""
    },
+    "HostOS": {
+      "description": "The RID used for restoring native OS dependent assets (i.e. RID specific binaries)",


It's still not clear to me from these two descriptions how this differs from RuntimeOS, we need to make that crisp.

ericstj · 2017-06-23T16:57:59Z

I think RuntimeOS=ubuntu.14.04;TargetRID=tizen.4.0.0-armel does not work for because that would restore ubuntu.14.04-armel in runtime.depproj where this must be tizen.4.0.0-armel.

Then to fix that we should make runtime.depproj honor the TargetRID/PackageRID (whatever name we end up using).

Bottomline I still think we only should have two concepts: what we're building on and what we're building for, today that's RuntimeOS and PackageRID respectively. I think each project should decide if it makes sense to use one or the other and behave cleanly in that way. If we can come up with a good distinction for a third concept then we can use it. I haven't yet seen a good reason for that distinction.

jyoungyun · 2017-06-26T03:41:53Z

When I built by the following command, it failed because there was no package for linux-armel.

docker run -it --rm -v $(pwd):/opt/code -w /opt/code -e 'ROOTFS_DIR=/crossrootfs/armel.tizen.build' hqueue/dotnetcore:ubuntu1404_cross_prereqs_v4-tizen_rootfs ./build.sh -buildArch=armel -release -RuntimeOS=ubuntu.14.04 -PortableBuild=false -- /p:OverridePackageSource=https://tizen.myget.org/F/dotnet-core/api/v3/index.json /p:TargetRID=tizen.4.0.0-armel

/opt/code/Tools/depProj.targets(83,5): error : Error no assets were resolved from NuGet packages. [/opt/code/external/runtime/runtime.depproj]
    0 Warning(s)
    1 Error(s)

Do you mean the RuntimeOS is what we're building on and the PackageRID(targetRID) is what we're building for ? Then, config.json should support 'PackageRID' option explicitly and runtime.depproj and ilasm.depproj should restore by 'PackageRID'. Please tell me if I need to check before merging it.

I'm sorry for late response.

ericstj · 2017-06-26T17:58:08Z

@jyoungyun there are still issues with the change that @tmds needs to resolve. I think as part of this change we should prescribe how to build for Tizen on an ubuntu/x64 machine so your test-case here is good validation.

Do you mean the RuntimeOS is what we're building on and the PackageRID(targetRID) is what we're building for ?

Yes

runtime.depproj and ilasm.depproj should restore by 'PackageRID'.

Partially correct.
runtime.depproj should consider a PackageRID override if specified since runtime.depproj isn't used for build-time execution, but for references from runtime facades and for test execution.
ilasm.depproj should restore by $(RuntimeOS)-<arch> (where <arch> here is the architecture of the build machine, I haven't looked into what the right property is, I think folks have been hardcoding x64) since we need to run ilasm during the build.

ericstj

Please address comments.

ericstj · 2017-06-26T19:03:00Z

src/SharedFrameworkValidation/RestoreSDKProject/RestoreSDKProject.csproj

    <RuntimeFrameworkVersion>$(_RuntimeFrameworkVersion)</RuntimeFrameworkVersion>
    <CoreFxPackageVersion>4.4.0-$(CoreFxExpectedPrerelease)</CoreFxPackageVersion>
-    <RuntimeIdentifier>$(_PublishRID)</RuntimeIdentifier>
+    <RuntimeIdentifier>$(TargetRid)</RuntimeIdentifier>


Looks to me like this was previously using the build-on-RID. @joperezr can confirm if it's OK to change this to the buiding-for-RID, and double check that you aren't missing anything by removing the global property passing that was here before.

ericstj · 2017-06-26T19:04:36Z

pkg/Microsoft.Private.CoreFx.UAP/uap.rids.props

 <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
-    <_aotSuffix Condition="'$(TargetGroup)' == 'uapaot'">-aot</_aotSuffix>
+    <PackageRID>$(TargetRid)</PackageRID>


I still see PackageRID used in multiple places where it is just set directly to TargetRID. A few points: why do the rename? are you sure you fixed every place that needs this remapping? why not just clean up PackageRID entirely (or not change it at all)?

tmds · 2017-07-10T10:00:09Z

@ericstj I rebased against master and compared the spec files for build.sh. The diff looks good.
I am unable to build allConfigurations on my Linux box, even not before this PR with the aot change. Perhaps this doesn't work on Linux?

You gave the suggestion to move the uap logic and the cross-target logic at the package and runtime.depproj respectively. They are at the toplevel now because they are used both for restoring and to name the package. Does that make sense?

weshaggard · 2017-07-12T21:12:32Z

dir.props

+
+    <_runtimeOS>$(RuntimeOS)</_runtimeOS>
+    <_runtimeOS Condition="'$(_runtimeOS)' == 'tizen.4.0.0'">ubuntu.14.04</_runtimeOS>
+    <RuntimeRID>$(_runtimeOS)-x64</RuntimeRID>


Why are you hardcoding x64 here?

Looks like this is more limited in scope then I had thought it looks like is only for our tools. We should switch to using the portable RID here and think about whether or not we want to hard code it to x64. If we do perhaps we should change this to be ToolRuntimeRID to indicate it is only about tools.

We should switch to using the portable RID here

Yes, when doing a portable build. When a maintainer does a distro specific build, he may want to use his own runtime to achieve that build.

think about whether or not we want to hard code it to x64

This value should be provided by BuildTools then (like RuntimeOS).

perhaps we should change this to be ToolRuntimeRID

I also think RuntimeRID doesn't convey the meaning well. To properly rename it, the change should also be made to the Buildtools and corefx config, since those generate/accept RuntimeOS.
To suggest another alternative: HostRID. (Buildtools could provide HostOS and HostArch.)

Yes, when doing a portable build. When a maintainer does a distro specific build, he may want to use his own runtime to achieve that build.

That is fine we would still guard it via the PortableBuild property just like we do in the other places. If it isn't portable then we will use the specific RID.

I still think the name Host is still a overloaded. I agree these are properties that should be set as part of tool initialization but it isn't that straight forward as an individual repo could chose to use different architectures for the CLI they use to build. I still like calling this ToolRuntimeRID as it more closely says what it is for.

weshaggard · 2017-07-12T21:15:21Z

I am unable to build allConfigurations on my Linux box, even not before this PR with the aot change. Perhaps this doesn't work on Linux?

We should probably file an issue to fix this s we want the package build to be able to happen on non-windows as well.

weshaggard · 2017-07-12T22:11:56Z

external/ilasm/ilasm.depproj

-    <NugetRuntimeIdentifier>$(RuntimeOS)-x64</NugetRuntimeIdentifier>
-    <!-- Tizen does not provide the x64 runtime so replaces it with the ubuntu.14.04-x64 -->
-    <NugetRuntimeIdentifier Condition="'$(RuntimeOS)' == 'tizen.4.0.0'">ubuntu.14.04-x64</NugetRuntimeIdentifier>
+    <NugetRuntimeIdentifier>$(RuntimeRID)</NugetRuntimeIdentifier>


@mellinoe it would appear that we used to depend on this being x64 all the time we need to ensure that is still the case after your changes.

Based on the change above (with your comment), it looks like it will still be x64. Technically we just need to restore an ILAsm that is compatible with the build environment, so an x86 version would/should work for Windows builds. But it's probably best and safest to keep it using the x64 version.

weshaggard · 2017-07-12T22:13:25Z

config.json

+      "description": "The RID of the target package.",
+      "valueType": "property",
+      "values": ["win7-x64", "ubuntu.14.04-x64"],
+      "defaultValue": "${RuntimeOS}-${ArchGroup}"


I suspect this default value isn't doing what you think it does. We don't support a generalized format string here it only supports a few custom strings.

Ah. I assumed this isn't used. The value I put here is the one that gets defaulted by the project. Should I clear it out?

I would just clear it out.

weshaggard · 2017-07-12T22:14:44Z

config.json

      "defaultValue": ""
    },
+    "PackageRID": {
+      "description": "The RID of the target package.",


What is the target package in this case? I assume you are implying that it is the RID of what you are building, however I see you a also using it in restoring dependencies.

Yes, this is the target RID you are building for.

weshaggard · 2017-07-12T22:15:47Z

external/runtime/runtime.depproj

-    <NugetRuntimeIdentifier Condition="'$(OSGroup)' == 'Unix' AND '$(RunningOnUnix)' != 'true'">linux-x64</NugetRuntimeIdentifier>
-    <NugetRuntimeIdentifier Condition="'$(TargetGroup)' == 'uap'">win10-$(ArchGroup)</NugetRuntimeIdentifier>
-    <NugetRuntimeIdentifier Condition="'$(TargetGroup)' == 'uapaot'">win10-$(ArchGroup)-aot</NugetRuntimeIdentifier>
+    <NugetRuntimeIdentifier>$(PackageRID)</NugetRuntimeIdentifier>


I'm not sure I understand why we are using PackageRID here? It should be based on the OS you are running on as these assets are primarily about running the tests on the build you are using.

Doesn't this runtime get included in the built package.
If I am building for 'banana' linux, I want to get&use the 'banana' runtime?

No the runtime doesn't get used for anything beyond running tests in this repo. It doesn't get included in any packages that we produce in corefx.

I see. I thought Microsoft.NETCore.App is built by corefx? This package contains the runtime?

That package is assembled in the core-setup repo, which consumes the runtime packages and the private corefx package (Microsoft.Private.CoreFx.NETCoreApp) but there are no packages that build in corefx that contain the runtime.

runtime.depproj pre-PR looks like it doesn't match the host but the target.
e.g.

<NugetRuntimeIdentifier Condition="'$(OSGroup)' == 'Windows_NT' AND '$(RunningOnUnix)' == 'true'">win-x64</NugetRuntimeIdentifier>

Yes that is a specific scenario where someone is trying to build for Windows while running on Unix.

I guess I did overstate when I mentioned that this is only used for testing. There are some scenarios were we use the assets for building the product in the case of a ReferenceFromRuntime reference (https://github.com/dotnet/corefx/blob/master/referenceFromRuntime.targets#L3) for projects that directly compile against System.Private.CoreLib. Unfortunately there are some slightly different APIs across the different OS's.

So the most common scenario is folks that want to build and run on the device the are on but in some cases folks will want to build for another runtime but not run locally but instead send them to another machine to run. So that is more of a target/PackageRID as opposed to Host/ToolRuntimeRID.

So with that said I think using PackageRID here is appropriate.

tmds · 2017-07-14T13:43:57Z

@dotnet-bot test Windows x64 Debug Build please

weshaggard · 2017-07-14T23:04:16Z

@tmds I would still like you to rename HostRID to ToolRuntimeRID to make the name a little more clear.

weshaggard · 2017-07-17T16:05:46Z

test UWP CoreCLR x64 Debug Build

weshaggard · 2017-07-17T16:07:54Z

@tmds Thanks a lot for all the work and iteration on the classifying the different type of RID's we have in our system. We truly appreciate you working through this with us and you are now one of experts with in comes to RID usage in corefx and .NET Core. Thanks for the contribution.

dnfclas added the cla-already-signed label Jun 14, 2017

karelz added the area-Infrastructure-libraries label Jun 15, 2017

karelz assigned tmds, mellinoe and weshaggard Jun 15, 2017

karelz modified the milestone: 2.1.0 Jun 15, 2017

weshaggard added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 15, 2017

weshaggard requested a review from ericstj June 15, 2017 15:57

ericstj reviewed Jun 15, 2017

View reviewed changes

config.json

"values": ["True", "False"],

"defaultValue": false

},

"RuntimeOS": {

Copy link

Member

ericstj Jun 15, 2017

Choose a reason for hiding this comment

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

This is meant to represent what we're running on.

ericstj reviewed Jun 15, 2017

View reviewed changes

ericstj reviewed Jun 23, 2017

View reviewed changes

ericstj suggested changes Jun 26, 2017

View reviewed changes

ericstj reviewed Jun 26, 2017

View reviewed changes

tmds added 10 commits July 10, 2017 09:50

Add HostOS tizen fallback

7934e18

Attempt to fix _portableRID

d64ea1c

PR feedback

4d7bc17

Fix Tizen build

1bb4d6c

PR feedback

22e9d82

comment-out cross-targetting

0f7d4e9

Fix Tizen build

fc18ae8

check how cross-target affects ci

85119e4

Fix OSX build

31f6398

Fix NETFX build

da2bf91

tmds force-pushed the external-no-portable branch from fe6e775 to da2bf91 Compare July 10, 2017 09:59

mellinoe removed their assignment Jul 11, 2017

weshaggard reviewed Jul 12, 2017

View reviewed changes

tmds added 2 commits July 13, 2017 15:51

Rename RuntimeRID to HostRID, use portable to build portable

f105cc9

clear PackageRID defaultValue

4196d22

HostRID -> ToolRuntimeRID

97ef94f

weshaggard approved these changes Jul 17, 2017

View reviewed changes

weshaggard merged commit 99809cd into dotnet:master Jul 17, 2017

weshaggard mentioned this pull request Jul 12, 2018

Adding basic support for an ilproj sdk. dotnet/arcade#317

Closed

eerhardt mentioned this pull request Aug 15, 2018

Updating S.R.CS.Unsafe to be built using the ilproj sdk. #31512

Merged

external/dir.props: remove Portable condition #21032

external/dir.props: remove Portable condition #21032

Uh oh!

Conversation

tmds commented Jun 14, 2017

Uh oh!

ericstj commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmds commented Jun 15, 2017

Uh oh!

ericstj commented Jun 15, 2017

Uh oh!

tmds commented Jun 16, 2017

Uh oh!

ericstj commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmds commented Jun 16, 2017

Uh oh!

ericstj commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyoungyun commented Jun 19, 2017

Uh oh!

ericstj commented Jun 19, 2017

Uh oh!

tmds commented Jun 23, 2017

Uh oh!

ericstj Jun 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!

ericstj Jun 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!

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

Uh oh!

Uh oh!

jyoungyun commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericstj commented Jun 26, 2017

Uh oh!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj Jun 26, 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!

tmds commented Jul 10, 2017

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!

weshaggard commented Jul 12, 2017

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

ericstj commented Jun 16, 2017 •

edited

Loading

ericstj commented Jun 16, 2017 •

edited

Loading

ericstj Jun 23, 2017 •

edited

Loading

ericstj Jun 23, 2017 •

edited

Loading

ericstj commented Jun 23, 2017 •

edited

Loading

jyoungyun commented Jun 26, 2017 •

edited

Loading

ericstj Jun 26, 2017 •

edited

Loading

weshaggard Jul 13, 2017 •

edited

Loading