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

Targeting mono #995

Merged
merged 32 commits into from
Dec 18, 2015
Merged

Targeting mono #995

merged 32 commits into from
Dec 18, 2015

Conversation

naveensrinivasan
Copy link

Targeting mono.

  • Build App
  • Run Unit Tests
  • Successfully build it on travis-ci
  • Run test's on travis-ci

naveen added 15 commits December 11, 2015 20:36
removed DocPlagiarizer

Fixed tests

removed unused variable

removed unused variable that was causing the build to fail

fixed the file name casing.

updated the nugget.exe to download System.Net.Http

PCL is required for mono

more fixes for travis
@shiftkey
Copy link
Member

/usr/lib/mono/xbuild/12.0/bin/Microsoft.Common.targets: warning : Reference 'NSubstitute, Version=1.8.0.0, Culture=neutral, PublicKeyToken=92dd2e9066daa5ca, processorArchitecture=MSIL' not resolved

Is package restore not working as expected?

@naveensrinivasan
Copy link
Author

@shiftkey OSX file system isn't case sensitive which is causing me an issue. So it works on mine but does not work on the linux.

Took care of NSubstitute.The issue was the NET40 was uppercase instead of net40 which is the actual case.

@naveensrinivasan naveensrinivasan changed the title [WIP] Targeting mono Targeting mono Dec 12, 2015
@shiftkey
Copy link
Member

And now I'm kinda curious why AppVeyor isn't reporting the status of this PRs...

- linux
install:
- curl -sS http://storage.bos.xamarin.com/bot-provisioning/PortableReferenceAssemblies-2014-04-14.zip > /tmp/pcl-assemblies.zip
- unzip /tmp/pcl-assemblies.zip -d /tmp/pcl-assemblies && mv /tmp/pcl-assemblies/PortableReferenceAssemblies-2014-04-14 /tmp/pcl-assemblies/.NETPortable
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need this, the PCL assemblies are preinstalled on Travis.

Copy link
Author

Choose a reason for hiding this comment

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

I tried this one also. It works on OSX but fails on linux for the same commit.

OSX - https://travis-ci.org/naveensrinivasan/octokit.net/jobs/96858329

linux - https://travis-ci.org/naveensrinivasan/octokit.net/jobs/96858330

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant https://travis-ci.org/naveensrinivasan/octokit.net/jobs/96850533.

If you look at the actual build log, the error there is from something completely unrelated (System.TypeInitializationException: The type initializer for 'System.Collections.Generic.List'1' threw an exception. ---> System.Threading.ThreadAbortException: during tests).

Copy link
Author

Choose a reason for hiding this comment

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

But it works on OSX and fails on linux. The same code. Do you know why?

And when I include PCL it does not fail every time.

Copy link
Contributor

Choose a reason for hiding this comment

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

PCL has nothing to do with it, that's just a coincidence (the random ThreadAbortException looks like a Mono bug).

Copy link
Author

Choose a reason for hiding this comment

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

OK. Thanks.

@shiftkey
Copy link
Member

@akoeplinger thanks for the review comments! ❤️

@naveensrinivasan
Copy link
Author

@shiftkey I am done with changes. There is a conflict on the README.MD

@shiftkey
Copy link
Member

@naveensrinivasan as this is targeting master, do you mind merging master in to address this conflict and verify the state of the mono build?

@naveensrinivasan
Copy link
Author

@shiftkey Will do that. Thanks

@naveensrinivasan
Copy link
Author

@shiftkey I resolved the conflict. The build failed 😫 in appveyor and linux because of StopsMakingNewRequestsWhenTakeIsFulfilled

@shiftkey
Copy link
Member

@naveensrinivasan ugh, feel free to mute that test and I'll open an issue to investigate

@naveensrinivasan
Copy link
Author

@shiftkey Thanks! Done!

@@ -90,7 +90,6 @@ public class TheGetAllForCurrentMethod
gitHubClient.Connection.Received(1).Get<List<Repository>>(thirdPageUrl, null, null);
}

[Fact]
Copy link
Member

Choose a reason for hiding this comment

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

Can you keep this [Fact] around with the Skip attribute set?

[Fact(Skip = "See https://github.com/octokit/octokit.net/issues/1011 for issue to investigate this further")]

@naveensrinivasan
Copy link
Author

@shiftkey Got it done! Thanks

@@ -2,9 +2,14 @@
<repositories>
<repository path="..\Octokit.Reactive\packages.config" />
<repository path="..\Octokit.Tests.Conventions\packages.config" />
<repository path="..\Octokit.Tests\packages.Octokit.Tests.config" />
Copy link
Member

Choose a reason for hiding this comment

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

@naveensrinivasan sorry, I should have been clearer here - these new entries are just duplicates of previous ones - can you 🔥 them to simplify the diff?

Copy link
Member

Choose a reason for hiding this comment

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

Eh, I'll clean this up after master goes green again.

shiftkey added a commit that referenced this pull request Dec 18, 2015
@shiftkey shiftkey merged commit 238b138 into octokit:master Dec 18, 2015
@shiftkey
Copy link
Member

@naveensrinivasan thanks again, outstanding stuff!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants