These are chat archives for Microsoft/CodeContracts

8th
Jul 2015
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 00:14
The issue was with roslyn-based compiler with static closure that was used by non-static closure. It's a bit complicated to express. But the tests are passing right now.

I need to figure out how to run the profiler on ccrewrite. Currently ccrewrite is 80% of my build time

This would be awesome. Let's create an issue and will try to solve it. I had this on my plate, because ccrewrite sloweness is a blocker for many other people.

Sam Harwell
@sharwell
Jul 08 2015 15:51
@SergeyTeplyakov I'm in meetings but i'll take a look at that issue right after
Sam Harwell
@sharwell
Jul 08 2015 16:41
@SergeyTeplyakov @mike-barnett How long should BuildRewriteRunFromSourcesRoslynVS14RC take to run?
Sam Harwell
@sharwell
Jul 08 2015 17:09
Looks like ~8 minutes
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:14
Yep, exactly. Around ~8 minutes. It seems that vbcsserver is not used by default
How did you build it? did you added nuget for new compiler?
Sam Harwell
@sharwell
Jul 08 2015 17:15
yes
I'm seeing some failures. The first failure was peverify is not part of the nuget package, so we'll presumably need another way to locate it.
worked around it by commenting out peverify in the tests (basically assume it works rather than verify)
In Roslyn_BuildRewriteRunFromSources, I'm seeing 4 failures
Sam Harwell
@sharwell
Jul 08 2015 17:20
All are returning -1 as the status code:
  1. OOBinterfacePost_74.rw.exe
  2. OOBInterfacePre_75.rw.exe
  3. OOBPost_77.rw.exe
  4. OOBPre_78.rw.exe
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:22
Sam, we can't disable PEVerify, this is super critical!
Let me just add those files to be able to move forward from short term perspective
Sam Harwell
@sharwell
Jul 08 2015 17:22
lol
I know, I just wanted to see if it was the only failure point
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:22
PEVerify will show if ccrewrite will screw up and provide invalid IL
OOB stuff: maybe that's related to the configuration itself.
can you show what the error is?
Sam Harwell
@sharwell
Jul 08 2015 17:23
I don't know how to
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:23
is it something like: can't find something? from csc
Sam Harwell
@sharwell
Jul 08 2015 17:23
Result3 Name:    Roslyn_BuildRewriteRunFromSources (Data Row 77)
Result3 Outcome:    Failed
Result3 Duration:    0:00:05.5819978
Result3 Message:    Assert.AreEqual failed. Expected:<0>. Actual:<-1>. J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\bin\Roslyn\..\..\..\..\packages\Microsoft.Net.Compilers.1.0.0-rc3\tools\OOBPost_77.rw.exe returned an errorcode of -1.
Result3 StackTrace:    
at Tests.TestDriver.RunProcess(String cwd, String tool, String arguments, Boolean mustSucceed, String writeBatchFile) in j:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\TestDriver.cs:line 132
   at Tests.TestDriver.Run(String targetExe) in j:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\TestDriver.cs:line 174
   at Tests.TestDriver.BuildRewriteRun(Options options) in j:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\TestDriver.cs:line 372
   at Tests.RewriterTests.Roslyn_BuildRewriteRunFromSources() in j:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\RewriterTests.cs:line 55
Ah, maybe this:
Test Name:    Roslyn_BuildRewriteRunFromSources (Data Row 77)
Test Outcome:    Failed
Result Message:    Assert.AreEqual failed. Expected:<0>. Actual:<-1>. J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\bin\Roslyn\..\..\..\..\packages\Microsoft.Net.Compilers.1.0.0-rc3\tools\OOBPost_77.rw.exe returned an errorcode of -1.
Result StandardOutput:    
Running 'J:\dev\github\sharwell\CodeContracts\packages\Microsoft.Net.Compilers.1.0.0-rc3\tools\csc.exe'
         /debug /t:exe /out:J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\bin\Roslyn\..\..\..\..\packages\Microsoft.Net.Compilers.1.0.0-rc3\tools\OOBPost_77.exe /d:CONTRACTS_FULL;ROSLYN;DEBUG;NETFRAMEWORK_4_5 /noconfig /nostdlib J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\TestHarness.cs  /r:J:\dev\github\sharwell\CodeContracts\Microsoft.Research\Imported\ReferenceAssemblies\.NetFramework\v4.5\mscorlib.dll /r:J:\dev\github\sharwell\CodeContracts\Microsoft.Research\Imported\ReferenceAssemblies\.NetFramework\v4.5\System.dll  J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\OOBPost.cs
Microsoft (R) Visual C# Compiler version 1.0.0.50616
Copyright (C) Microsoft Corporation. All rights reserved.

Rewriting 'J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\bin\Roslyn\..\..\..\..\packages\Microsoft.Net.Compilers.1.0.0-rc3\tools\OOBPost_77.exe' to 'J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\bin\Roslyn\..\..\..\..\packages\Microsoft.Net.Compilers.1.0.0-rc3\tools\OOBPost_77.rw.exe'
Running 'J:\dev\github\sharwell\CodeContracts\Foxtrot\Driver\bin\debug\foxtrot.exe'
         /out:J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\bin\Roslyn\..\..\..\..\packages\Microsoft.Net.Compilers.1.0.0-rc3\tools\OOBPost_77.rw.exe -nobox -libpaths:J:\dev\github\sharwell\CodeContracts\Microsoft.Research\Imported\ReferenceAssemblies\.NetFramework\v4.5 /libpaths:J:\dev\github\sharwell\CodeContracts\Microsoft.Research\Contracts\bin\Debug\.NETFramework\v4.5;J:\dev\github\sharwell\CodeContracts\Microsoft.Research\RegressionTest\ClousotTestHarness\bin\debug\.NETFramework\v4.5  /throwonfailure /rw:OOBPost_77.exe,TestInfrastructure.RewriterMethods J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\bin\Roslyn\..\..\..\..\packages\Microsoft.Net.Compilers.1.0.0-rc3\tools\OOBPost_77.exe
Microsoft (R) .NET Contract Rewriter Version 1.8.10107.10
Copyright (C) Microsoft Corporation. All rights reserved.

elapsed time: 805.4139ms
Running 'J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\bin\Roslyn\..\..\..\..\packages\Microsoft.Net.Compilers.1.0.0-rc3\tools\OOBPost_77.rw.exe'

Expected: Postcondition, Actual: Assert
Expected: Postcondition, Actual: Assert    at TestInfrastructure.Assert.AreEqual(Object expected, Object actual) in J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\TestHarness.cs:line 104
   at Tests.Sources.TestMain.NegativeTest() in J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\TestHarness.cs:line 81
   at Tests.Sources.TestMain.Main() in J:\dev\github\sharwell\CodeContracts\Foxtrot\Tests\Sources\TestHarness.cs:line 34
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:24
ok
now it is much better
Sam Harwell
@sharwell
Jul 08 2015 17:25
Re peverify - do we need different versions of peverify or can we just always use the one from the current version of the .NET framework installed on the system?
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:26
we can but in this case we have to change our testing code.
I'll update PR shortly...
Sam Harwell
@sharwell
Jul 08 2015 17:26
perhaps it wouldn't be a big change
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:26
let's fix this in step by step manner. There is a tons of other improvements that are required
Sam Harwell
@sharwell
Jul 08 2015 17:27
yes
I can easily resolve the peverify issue. I don't know why this test is failing though.
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:28
I need to reproduce this first...
Sam Harwell
@sharwell
Jul 08 2015 17:30
Git has a command to clean your working directory. But it will delete any files you didn't commit (even if the reason you didn't commit them is they were in .gitignore).
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:30
yep. my current working copy is ready for testing...
Sam Harwell
@sharwell
Jul 08 2015 17:32
Here's what I did:
  1. Open CodeContracts.sln in Visual Studio 2013
  2. Navigate to RewriterTests.cs
  3. Run Build → Build FoxtrotTests10
  4. Wait for Test Explorer to show tests
  5. Run Roslyn_BuildRewriteRunFromSources (single test) from test explorer
Note that I have the following changes from your branch:
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:32
I'm looking into the OOBPost.cs... and I can't get why NegativeExpectedKind is a Postcondition...
Thanks. I'm running it right now. I've spend days on this stuff. I have a general understanding how to run my tests;)
Sam Harwell
@sharwell
Jul 08 2015 17:33
  1. I created the file .nuget/packages.config:
     <?xml version="1.0" encoding="utf-8"?>
     <packages>
       <package id="Microsoft.Net.Compilers" version="1.0.0-rc3" />
     </packages>
  2. Replace CreateRoslynOptions("VS14RC3") in RewriterTests with the following:
     CreateRoslynOptions(@"..\..\..\..\packages\Microsoft.Net.Compilers.1.0.0-rc3\tools")
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:34
Just take a look at Foxtrot\Tests\Sources\OOBPost.cs ...
Sam Harwell
@sharwell
Jul 08 2015 17:35

Thanks. I'm running it right now. I've spend days on this stuff. I have a general understanding how to run my tests;)

I've seen tests fail because they were run in a different order through manual selection. I was just making sure we are exactly on the same page in case you aren't able to reproduce otherwise. :smile:

I'm in that file
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:36
This test should always fail. Why with "behave" equals to "false" we should see a precondition violation?
Am I missing something?
Ok. Test is passing in my case...
ok. I know why postcondition violation should happen here...
so there is a postcondition for the NameObjectCollectionBase and it seems that in my case I'm using this contracts and in your case those contracts are absent
I was confused by IFoo and IFooCotnracts here but they're irrelevant
Keys property has implicit postcondition that Keys are never null
Sam Harwell
@sharwell
Jul 08 2015 17:41
Maybe the diference in ToolsPath is the difference
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:41
I don't know...
There is absolutely crazy logic in this "testing framework". It would be very useful to refactor this.
But that one of the reasons why I don't think that switching to nuget compiler is enough. All this stuff requires redesign and reconsiderations...
Sam Harwell
@sharwell
Jul 08 2015 17:42
There are additional options to avoid space problems.
One is using NuGet, but make the first step of CreateRoslynOptions copy the binaries from the NuGet packages directory to the Rosyln\VS14RC3 folder
Also an important note - if you copy peverify that was in VS14RC over to VS14RC3, it will not increase the repository size.
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:46
I did exactly like that. I moved PEVerfiry from VS14RC to VS14RC3
Sam Harwell
@sharwell
Jul 08 2015 17:47
Any files you just moved, I have no objection to checking in (right now they are absent I believe due to gitignore)
The only ones I want to rethink (because you can't rethink it later) is new binaries
The only once I want to rethink (because you can't rethink it later) is new binaries
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:50
I know. But we need to find out a way to rethink this. Just because current repo contains few megs of binaries.
And currently the only new binaries that I've pushed are: csc.exe, vbc.exe and VBCSCompiler.exe. Other assemblies are the same as they were in VS14RC folder. So I increased size of the repo by 146K
Sam Harwell
@sharwell
Jul 08 2015 17:52
I didn't realize the other assemblies are the same (I can't see them, so I assumed you use the full set from rc3)
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:53
I've checked before push
And can you try to pull from my branch to test it?
Sam Harwell
@sharwell
Jul 08 2015 17:54

csc and vbc are in the nuget package I mentioned. If it is easy to reference them I think we should.

I looked thorugh the history is pretty good detail previously. I don't think we need to rewrite history to remove old objects. most of the space is items that aren't easily removed.

testing now
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 17:57
ok..

I looked thorugh the history is pretty good detail previously. I don't think we need to rewrite history to remove old objects. most of the space is items that aren't easily removed.

That's bad. But it is what it is..

Sam Harwell
@sharwell
Jul 08 2015 17:58
8 minutes :watch:
Remember that the checkout is ~900M, but the repository with history is only ~240M (or something close to that). When the same binary exists in multiple locations, it is automatically shared in the history storage.
So cloning isn't as bad as one might think.
Have to run to lunch in a bit. Will wait for the test though
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 18:02
ok. Yep, I'm aware about git ability to reuse the same binaries...
Sam Harwell
@sharwell
Jul 08 2015 18:06
Same 4 errors
could be due to me not building the entire solution?
Not sure, need to get some lunch and then I'll try again
Note that I updated to your latest commit and reverted my local changes for the previous test, so it wasn't failing due to nuget stuff
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 18:17
Usually I'm trying to rebuild all fist (especially after switching branches)
Have a good lunch! Ping me when you would be ready to investigate this.
Sam Harwell
@sharwell
Jul 08 2015 19:41
What configuration/platform are you using in Visual Studio?
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 19:43
Mixed Platforms
Debug/Release
are you still getting the same error?
Sam Harwell
@sharwell
Jul 08 2015 19:50
rebuilding debug/mixed platforms
and this should work running a single test from VS test explorer, right?
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 19:55
Yep, all FoxtrotTests10 should work correctly
from VS test explorer
Sam Harwell
@sharwell
Jul 08 2015 20:22
After a rebuild, it passed.
Must be the project dependencies are not set correctly (perhaps an indirect reference to build outputs that aren't specified as a project reference)
Key here being it passed
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 20:22
msbuild sucks :)
I saw this for a few times when I'm switching branches
and I would appreciate for quick look into another PR
Sam Harwell
@sharwell
Jul 08 2015 20:23
msbuild works if you declare your dependencies properly ;)
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 20:23
Sam Harwell
@sharwell
Jul 08 2015 20:23
you meaning "anyone", not you specifically
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 20:23
adding test case and will push it to master

you meaning "anyone", not you specifically

Unfortunately, I meant you specifically :( I'm asking @mike-barnett to look at any changes in the logic, but last PR from myself is formatting

I would have parental leave very soon (this week, I think). So I would like to publish beta release of the tool before that...
because during parental leave I'm not sure that I would have time to work on major fixes for the next few weeks
Sam Harwell
@sharwell
Jul 08 2015 20:25
I meant "MSBuild works when [people] declare [their] dependencies properly". Meaning I wasn't blaming you for the fact that CodeContracts currently doesn't.
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 20:25

I meant "MSBuild works when [people] declare [their] dependencies properly". Meaning I wasn't blaming you for the fact that CodeContracts currently doesn't.

Agreed:)

I'm working in the MSBuild team, actually :)
Sam Harwell
@sharwell
Jul 08 2015 20:26
#93 won't work as a pull request really until #91 is merged.
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 20:26
sure
I know
Sam Harwell
@sharwell
Jul 08 2015 20:26
Are you sure we won't need to update the compiler again before this release?
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 20:26
I think I can push #91, because your tests are passing and Mike signed off this stuff
no
sorry. Yes, I'm sure
Sam Harwell
@sharwell
Jul 08 2015 20:27
ok, I'm good with #91
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 20:27
RC3 is super close to RTM version as far as I know (from backward compatibilty perspective)
Sam Harwell
@sharwell
Jul 08 2015 20:28
Can you close #93 and then send it again (exactly the same pull request)?
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 20:33
yep. working on it
Sam Harwell
@sharwell
Jul 08 2015 20:34
You are missing copyright headers in some of the added files. I recommend using the two-line copyright header instead of the full one:
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
No need to change existing ones, but for new files (e.g. when you split files up), you should add this
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 20:38
Microsoft/CodeContracts#94
I am. Will add them all
Sam Harwell
@sharwell
Jul 08 2015 20:45
@SergeyTeplyakov hang on
Pull request #94 didn't go as planned
All of the highlighted commits here are duplicates:
blob
You need to force-push commit 125ffc6ea436c9928d1d7b13fce09f802cf10cac to your fork's branch features/ReformatFoxtrot
You don't need to do anything to pull request #94
just make sure to force push to your fork, and not to the Microsoft/CodeContracts repository
When you force push, it will be automatically corrected
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 20:52
ok. Let's double check that we're on the same page: current fork has duplicate commits (because I rebased with master but not merged back).
Currently my working copy is up-to-date...
so what exactly should I do with my fork? (sorry for stupid questions:( )
Sam Harwell
@sharwell
Jul 08 2015 20:53
No problem
currently your local copy has a branch features/ReformatFoxtrot at commit 1bf5601f2519dbc76275284844abc873224c7510
is that correct?
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 20:55
my local copy's head is at 68bb4026b4eb7e427e3ae63eb98e00f3f97a7744
just because I've fixed copyrights
but that was minor fix I can easily switch to 1bf56 to fix double commits
and replay few new commits on top of new correct head
Sam Harwell
@sharwell
Jul 08 2015 20:58
Two possible options:
  1. Interactive rebase features/ReformatFoxtrot onto 125ffc6ea, and only pick the last 3 (split program, reformat, and copyright)
  2. Add me as a collaborator to your fork and I can rebase/update the branch
I actually already did the rebase locally and verified that the resulting commit is identical to your commit 68bb402
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 21:00
Thanks for option 2. Let me try to use option 1 for next 10 minutes. It should be enough. Then we'll double check and decide

I actually already did the rebase locally and verified that the resulting commit is identical to your commit 68bb402

Does it mean that if I'll add you as a collaborator you can just force push and I can merge?

Sam Harwell
@sharwell
Jul 08 2015 21:02
If you give the go ahead, you can merge #94 in less than 30 seconds
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 21:03
by "go ahead" you mean...
Sam Harwell
@sharwell
Jul 08 2015 21:04
meaning I won't ever push to your fork unless you say "ok please push to my fork"
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 21:04
ok understood
I'll give you an access
Sam Harwell
@sharwell
Jul 08 2015 21:05
want me to push it?
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 21:06
I guess I've added an access...
Sam Harwell
@sharwell
Jul 08 2015 21:06
you have
still have to say yes though
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 21:07
hm... Could you point me out where? :(
Sam Harwell
@sharwell
Jul 08 2015 21:07
here, confirm you want me to push
here = on gitter chat
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 21:08
yep. Confirmed:))
oh.. sure
Sam Harwell
@sharwell
Jul 08 2015 21:08
Done. Pull request #94 no longer has duplicates, you can merge it using the GitHub UI
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 21:08
I would be out for next 20-30 minutes. Another meeting. After that I would work on two remaining issues.
Done
Thanks a lot!
Sam Harwell
@sharwell
Jul 08 2015 21:09
:+1:
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 21:47
Ok. One PR to merge and another bug to fix and we're ready for the new release!
Sam Harwell
@sharwell
Jul 08 2015 22:05
Is #18 fixed now?
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:05
Technically, I need to fix cccheck caching issue...
Sam Harwell
@sharwell
Jul 08 2015 22:06
#80 is fixed I believe. The commit message says "Fix for #80", but since that doesn't match the pattern "Fixes #80" it did not cause the issue to close automatically.
Wait lol
#80 is part of PR #81, which is not merged.
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:07
tom-englert proposed some other changes to that PR
Sam Harwell
@sharwell
Jul 08 2015 22:07
However, after #81 is merged then what I said previously is correct (#80 won't close automatically due to the syntax)
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:07
I've created a fix, but didn't merge PR yet.
Sam Harwell
@sharwell
Jul 08 2015 22:07
K
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:07
yep
I hope to close this one and #55 today
Sam Harwell
@sharwell
Jul 08 2015 22:08
I'll look at #55
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:09
I've added a test for this and fighting with git
Sam Harwell
@sharwell
Jul 08 2015 22:09
Rebase might not work
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:09
The fix looks good, but lacks a test
Sam Harwell
@sharwell
Jul 08 2015 22:11
I was able to do a clean merge of #55 locally
without a rebase
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:12
cool
Sam Harwell
@sharwell
Jul 08 2015 22:13
where is the test case (which branch)?
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:13
sec
I'll publish it right now
0fcf4476b0031837c3fd62c0dbd132e46768dc7c
Sam Harwell
@sharwell
Jul 08 2015 22:23
want me to create a new pull request that can be easily merged, containing both #55 and your test?
I was successful in rebasing #55
Or you can
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:24
sure. it would be very useful
Sam Harwell
@sharwell
Jul 08 2015 22:24
Can I push to SergeyTeplyakov/hubuk-issue20? Then you send the pull request using GitHub UI
gah nevermind
It needs a change, I will just make the change and submit the PR
it'll have 3 commits, one from @hubuk, one from @SergeyTeplyakov , and one from @sharwell lol
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:30
lol
ok
Sam Harwell
@sharwell
Jul 08 2015 22:31
Submitted pull request #95
I formatted the code as part of your commit, but then added a commit to fix an incorrect comment.
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:31
sure
Sam Harwell
@sharwell
Jul 08 2015 22:31
(since you formatted the rest of the foxtrot code, I figured this should match)
You can close #55
Probably safe to close #75 as resolved
Any reason not to merge #57?
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:40
Merged #57, closed other two
Sam Harwell
@sharwell
Jul 08 2015 22:44
#43 can be merged
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:50
Done.
Sam Harwell
@sharwell
Jul 08 2015 22:51
I don't understand #39, but I don't like that it's another binary going in. :worried:
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:53
Yep, I can't get this PR as well.
Sam Harwell
@sharwell
Jul 08 2015 22:53
I'm going to rebase and resubmit #46 so it can be merged
@tom-englert confirmed this issue as well
I'm not ready to include everything from the larger commit #84 or #85, but #46 should be easy since it's very small and targets a very specific bug.
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:56
Yep. I totally agree about #84 and #85. The impact of them could be quite significant actually
I'll merge #46 then
Sam Harwell
@sharwell
Jul 08 2015 22:57
#46 has conflicts that I'm fixing
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 22:57
understood
I meant I'll merge after that
Sam Harwell
@sharwell
Jul 08 2015 23:01
#96
That leaves the following:
  • #46 can be closed (superseded by #96)
  • I don't know what to do about #54 yet, but I'm concerned that it claims to touch 20,000 lines of code; probably defer dealing with it for next release
  • #81 ( :question: where are we at on this one)
Sergey Teplyakov
@SergeyTeplyakov
Jul 08 2015 23:06
About #54: I think the reason why it has so many changes is because the code was reformatted as well as changed.
We can propose to split the fix, otherwise it is very hard to tell is the fix correct or not
Working on #81, will push an update shortly
and my team mates fixed Contracts.Targets for MSBuild v12, I'm going to publish PR with their fix
Sam Harwell
@sharwell
Jul 08 2015 23:11
:sunglasses: