Some fixes. #219
Some fixes. #219
Conversation
|
Do you have tests for the fixes ? |
| kits_suffix = "bin" | ||
| if PLATFORM == "x64": | ||
| kits_suffix += r"\x64" | ||
| kits_suffix = os.path.join("bin",PLATFORM) |
There was a problem hiding this comment.
this should be a separate pull request, since this is unrelated commit from someone else:
There was a problem hiding this comment.
note that #194 is linked with other pull requests. @tonyroberts is going to merge these changes once appveyor CI gets fixed.
|
From looking at the commit titles, it looks like 3 commits its about right. Seems to be addressing 3 different issues. in-lieu of doing 3 separate |
|
also need to downgrade your syntax to C# 4.0 |
|
@denfromufa Yes I agree with all points. I just wanted to quickly show up the problems. It could take me a while to clean this up as I'm trying to work on something else (I just figured it would be a good idea to let people know there are some fixes available). If someone wants to step in until then feel free to do so. If you encounter problems while reproducing feel free to ask. I'm pretty sure everything can be reproduced with C# as well. Is there a strong reason to stay on C# 4? |
|
By the way after all these fixes I still see in the build output. So there is still something wrong. As I used a VS developer prompt everything should be available. But I didn't investigate as the build continues and the package is available in |
|
@matthid c# 6 syntax should be downgraded to c# 4 due to .NET 4.0 and Mono compatibility. |
|
Let me first say: I have no problem with downgrading (once I have some time... the project looks a bit dead anyway as there are quite some outstanding PRs). But IMHO those are the wrong reasons. You should ensure mono and net40 compat though build and test. There is nothing stopping you from compiling c#6 to net40. Only a small subset is not working because of missing .net classes but even those you can get working by defining the missing classes yourself (if really needed). Just google for c#6 and net4 to get sources for this. The travis build is green, which I would say means it works. Btw later mono versions only run with net45 anyway. |
|
@matthid I read about partial compatibility of C# 6 with .NET 4.0, but did not know that Mono defaults to C# 6. However looks like their main documentation is outdated, which is where I looked first: http://www.mono-project.com/docs/about-mono/languages/csharp/ C# 6 is not ready in Mono compiler according to that page. WRT pythonnet slow development cycle - please note that it is still more alive development than IronPython and the codebase is older than both IronPython and F#! |
|
@denfromufa Yes Thanks, and the code worked great for me (after the bugfixes at least). I tried IronPython first and couldn't make it do anything :) Yeah the page is really outdated. Mono moved fast after MS open sourced a lot of things... I think they use Roslyn now and can evolve as fast as Visual Studio... |
|
I cherry picked the original commit from #194 this should make it possible for you to merge both PRs, and it makes it easy to use my branch for other things. Just to be sure you should merge #194 first (but git should be able to handle that just fine). Additionally I added test for 2823941. Now I'm looking into test for 35cd5fa. I think cc8b796 is quite hard to test... |
|
I'm not using python that much, but as far as I understand it I had to add a module to be able to use relative imports. I hope its fine the way I did it, please review. Edit: http://stackoverflow.com/questions/16981921/relative-imports-in-python-3 |
|
Maybe a stupid question: But where are the |
|
@matthid that is a good point! embedding tests are not run on travis or appveyor. Once I did manage to run them locally and passed, but there is something brittle there with importing file paths: https://github.com/pythonnet/pythonnet/blob/master/src/embed_tests/pyimport.cs#L27 |
|
I opened a separate issue for embedding tests: #224 |
|
Ok I managed to reproduce the problem with the "regular" test suite, but I think it might make more sense to add it to the embedding tests (the code might be a bit simpler there). I think this PR is now ready to review and merge. What about C#6. Should I still change to C#4? |
|
To test that the test cases do indeed reproduce the problems you can use my add_tests branch. Then cherry-pick the fixes to make the tests work one by one. |
|
@matthid travis CI is failing to build your current pull request: |
|
Yep investigating, locally its working :( |
No problem, as long as both travis (looks good) and appveyor (have not seen yet) are able to build this. |
|
might be a mono bug, an appveyor run might be cool here to clarify |
|
Yep its a mono bug: public class TestConvert : System.Dynamic.DynamicObject
{
public override bool TryConvert(System.Dynamic.ConvertBinder binder, out object result)
{
result = null;
return true;
}
}
public class Test : System.Dynamic.DynamicObject
{
public override bool TryInvokeMember(System.Dynamic.InvokeMemberBinder binder, object[] args, out object result)
{
result = new TestConvert();
return true;
}
public static void TestMethod()
{
dynamic t = new Test();
string result = t.SomeMethod();
}
}Test.TestMethod(); // Works on .NET, crashes on mono |
|
We could workaround here at pythonnet by checking for Pythons |
|
Done. |
|
You can configure a appveyor account to run off your github account. That On Saturday, June 4, 2016, Matthias Dittrich notifications@github.com
|
|
@vmuriart Yeah, thanks! But I'm testing on windows already locally. Just mentioned it that it would be nice for others to see (because everything was red above)... |
|
I've cherry picked "Alternative Fix for #182" into master. Thanks! |
| { | ||
| if (pyObj != null) | ||
| { | ||
| if (pyObj.obj == Runtime.PyNone) |
There was a problem hiding this comment.
Doesn't this alter the behaviour? Previously a PyObject wrapping None would be returned, but now that's replaced with null? I must be missing something...
There was a problem hiding this comment.
Yeah you are right, it is a workaround that you are not able to return null via dynamic conversion (=later) on mono. please see the linked bug report as well. Feel free to ask if you want a code example (could take some time though)
There was a problem hiding this comment.
Yeah I get that, but here you are returning null where previously an object (PyNone) was returned. Could function just not return pyObj without checking for None? That's the bit I don't understand.
There was a problem hiding this comment.
you could and even should imho, but then you are triggering the mono bug :). The problem is later. when you save such a none object in a dynamic and want to resolve it to a string (for example). This will not work as long as the mono bug exists. In fact one of my tests discovered this glitch
There was a problem hiding this comment.
Ah ok, that was the bit I was missing - that the bug was triggered later when dealing with the None PyObject instance.
… (maybe C# interactive as well).
Ok, but you could have just told me to split the PR or tell me what the problem was with the whole PR (didn't see that there are conflicts until now, because I didn't saw that on the phone). |
|
apparently in python 2.7.12 some importing issues have been fixed in C-API: |
|
@denfromufa I have no idea what you are trying to tell me or what I should do now with this PR :) |
|
Okay, decision time: Do we allow C#6? Is there any compelling reason not to, as the tests are working? In my opinion, this PR can be merged. |
|
I thought @tonyroberts cherry-picked this pull request? C# 6.0 is supported by both CI, so I'm +1 on adopting this! On Thursday, July 14, 2016, Benedikt Reinartz notifications@github.com
|
|
@denfromufa Only parts, not sure whats wrong with the rest. I can only improve this when I know whats missing... |
|
@denfromufa @vmuriart Merge? |
|
@filmor can you give me one day to test this locally? I would like to make On Fri, Jul 22, 2016 at 3:33 AM, Benedikt Reinartz <notifications@github.com
|
|
ok, tested locally and it is working! great finding and fix! |
Fixes a bunch issues I encountered when trying to implement https://github.com/matthid/googleMusicAllAccess.
Note: this includes #194 (because I needed the fix), let me know when you merge #194 so I can rebase and remove the last commit.