Make python bindings article partially work on win #157

Znunu · 2020-10-18T23:16:03Z

This PR fixes some of the problems that I brought up in issue 151

jima80525 · 2020-10-20T01:41:40Z

@Znunu
Hey! Nice work. I'm pulling this down presently and testing it out.
It looks like the CI build failed. I'm not sure if you've got a circleCI account (you can use your github acct as the login), but it's pretty easy to set up. Let me know if you have issues. (those are all formatting issues, btw)

More significantly, the CFFI build is failing on (at least) python3.7. CFFI really doesn't like #ifdef directives. Looking at it, this makes some sense as it can't know what the value of that is until compilation time. :(

Does the CFFI portion work on Windows? We might need to think some more about the best way to present this.

Znunu · 2020-10-20T20:52:14Z

CFFI didn't work for me either. tbh I didn't intend to make that portion portable, but I didn't realize I broke it, so I went ahead and tried anyway. Like you mentioned, it didn't like the directives, so I added some code that filters them out.

Znunu · 2020-10-29T22:23:59Z

Hello?

jima80525 · 2020-10-30T01:54:33Z

Hey! @Znunu - this got away from me. I'll take a look this weekend.

Thanks for the ping!

jima80525

@Znunu
This is getting close! I left a couple of comments and some fixes to get linux back and running.

THANK YOU!!!!

and, again, thanks for the ding on this last week. Been getting behind on things. :)

jima80525 · 2020-10-31T20:57:28Z

python-bindings/overview_article/tasks.py

+        print_banner("Building C Library")
+        cmd = "gcc -c -Wall -Werror -fpic cmult.c -I /usr/include/python3.7"
+        invoke.run(cmd)
+        invoke.run("gcc -shared -o cmult.so cmult.o")


you really want this to be -o libcmult.so

oh - I see the problem now. I"ll suggest a change in ctypes_test.py to work around this.

Now reviewing my comments this looks strange - these two were the first comments I made getting things to work. Then I went back and did the full review. :)

jima80525 · 2020-10-31T21:05:55Z

python-bindings/overview_article/ctypes_test.py

-    c_lib = ctypes.CDLL(libname)
+    lib_ext = ".dll" if sys.platform.startswith("win") else ".so"
+    libname = pathlib.Path() / f"cmult{lib_ext}"
+    c_lib = ctypes.CDLL(libname.resolve().__str__())


Linux and mac users are really gong to expect libcmult.so. I'd recommend doing this instead of line 9:

if sys.platform.startswith("win"): libname = "cmult.dll" else: libname = "libcmult.so"

and then just using libname instead of the fstring on line 10.

jima80525 · 2020-10-31T21:08:21Z

python-bindings/overview_article/tasks.py



-@invoke.task(build_cmult)
+@invoke.task()


Is the build on windows taking significant time? If not, I'd prefer to leave the dependency here. It makes the individual targets a bit more fool-proof.

No, it doesn't. On windows, the problem is that it'll only build if we supply the argument pointing to visual studio.

ahhh. I get it. That's fine. I missed that point.

jima80525 · 2020-10-31T21:10:16Z

python-bindings/overview_article/cffi_test.py

@@ -1,4 +1,5 @@
 #!/usr/bin/env python3
+# IDE might complain with "no module found" here, even when it exists


This is because it doesn't exist when you start out. :) CFFI creates the module when you run the build.

Yeah, but my IDE, PyCharm, didn't want to acknowledge it, even after making it. I actually lost a lot of time figuring out what the problem is, since I didn't even bother to try and run it, lol

that's odd. I wonder what pycharm is doing, but not enough to actually go find out, you know. :)

jima80525 · 2020-10-31T21:21:23Z

python-bindings/overview_article/tasks.py

+    overview article.
+
+    If anything is causing trouble get in touch at
+    Znunu @ github or VimVim @ fcZBB2v @ discord"""


I appreciate you taking ownership for your work, but I'd propose rephrasing this to:
"Special thanks to Znunu @ github for working through the Windows compatibility issues."

(which, I know, would sound if you said it, but it will sound like it's coming from me when we merge it :) )

It's not so important for me to be credited. Just wanted to add a quick and easy way for someone to get in touch. Someone might encounter a bug or have a problem, and thus open the file. Maybe I'm doing this the wrong way. I can remove it again.

I've got a question in with Dan to see what he wants. I'll let you know when he gets back to me (I've set a reminder that if I don't hear from him by Tuesday night, I'll ding him again.)

jima80525 · 2020-10-31T21:22:41Z

python-bindings/overview_article/tasks.py

+        else:
+            c.run("rm -rf {}".format(pattern))
+    if on_win:
+        c.run("rmdir /s /q Release >nul 2>&1")


That's nice.

Maybe I'm missing something but could we replace those delete/rm commands with something Python-native that works across platforms?

E.g. https://stackoverflow.com/questions/6996603/how-to-delete-a-file-or-folder

Doh! Can't believe I missed this. Thanks, @dbader

jima80525 · 2020-10-31T21:26:53Z

python-bindings/overview_article/tasks.py

+    if on_win:
+        invoke.run("python cffi_test.py")
+    else:
+        invoke.run("python3 cffi_test.py", pty=True)


I think this could be

invoke.run("python cffi_test.py", pty=not on_win)

straight up python will work on the other systems. I was just being pedantic. :)

Znunu · 2020-11-01T16:12:57Z

Uhhhh.... I hope I'm doing this review thing correctly
Anyway, I fixed some things you suggested, and added comments at the rest!

jima80525 · 2020-11-01T22:07:57Z

You're doing great on the review! No worries on that front - any shortcomings are from my slow response time. :)

jima80525 · 2020-11-01T22:13:16Z

Doh! One more thing I just found - it looks like you checked in cython_example.pyx which is a generated file. Can you please do a git rm on it? Otherwise you get odd messages when you do invoke clean that you've modified the code tree. :)

Never mind that comment. Turns out I checked cython-example.pyx and it needs to be there. Looks like your clean task mods went a little too far!

jima80525 · 2020-11-01T22:30:15Z

python-bindings/overview_article/tasks.py

+        "*.dll",
+        "*.exp",
+        "*.lib",
+        "*.pyx",


Aha! This is a problem. I missed this one at first. The *.pyx file is actually hand-build and need to NOT be removed.

This causes the invoke build-cython test-cython sequence to fail.

jima80525 · 2020-11-01T22:34:04Z

This is super close! I think fixing the clean target and then getting feedback from Dan on the other comment and we should be done.

THANK YOU again for this work! I really appreciate it and I hope you've learned some! Sounds like this is your first time through a PR process? If so, you're doing great! (actually, you're doing great even if it's NOT your first time, but that's be pedantic.)

Znunu · 2020-11-02T00:25:07Z

No problem, I'm happy to contribute and appreciate your cooperation :)
Well, technically this isn't my first PR. Was working on something with a friend. For hacktoberfest we switched from just committing, to merging each others PRs ^_^"

jima80525 · 2020-11-02T01:44:10Z

This looks good!

jima80525 · 2020-11-04T19:25:03Z

Hey - I heard back from Dan - He wants us to pull the reference to you in the tasks.py and add a shout out to you in the README.md. There ISN'T a readme in this directory (shame on me), so I'll propose this:

you remove the reference in tasks.py
I'll get the PR merged
I'll add a README.md file for this and call you out. Do you want me to use your full name, or just znunun at github?

Thank you again for the hard work on this! Dan was impressed that we have a PR!

jima80525 · 2020-11-05T02:28:34Z

@Znunu - I went ahead and wrote a README.md for the directory. It might be easier (in terms of git merging etc) if you just add it.

Here's the text:

# Real Python - Python Bindings Sample Code Repo

This is the repo to accompany the [Python Bindings](https://realpython.com/python-bindings-overview/) article.

To be able to run the code, you must first install the requirements:

```console
$ python -m pip install -r requirements.txt

This should be done inside a virtual environment.

Once that is installed, you can use the invoke tool mentioned in the article to build and run the tests. See the tasks.py file or run invoke --list to get more details.

Special thanks to "znunu at github.com" for getting these examples running on a Windows platform!


If that's a hassle, I can do it after we get this merged. :)

Znunu · 2020-11-06T16:13:06Z

I added the readme. Thanks for writing one! Removed the reference from both tasks.py and readme.md, don't care about being credited. Tried to add Dan's suggestion as well

jima80525

LGTM!

jima80525 · 2020-11-08T14:58:50Z

Great work on this, @Znunu - I really appreciate you stepping up and helping!

Is partially windows compatible

1592379

Znunu added 5 commits October 20, 2020 22:05

cffi part works on windows

2d14255

what kind of tiny screens do you all use?

38a3852

pleases the linting overlords

2a058b0

third time's the charm

4ea1d0f

removed a space

fbe3d70

Znunu and others added 2 commits October 20, 2020 23:09

formatted?

507b07c

Merge branch 'master' into master

fc9ad84

jima80525 requested changes Oct 31, 2020

View reviewed changes

Znunu added 3 commits November 1, 2020 16:59

renamed shared lib

82d33bb

Merge remote-tracking branch 'origin/master' into master

8a04327

removed import

c097898

Znunu changed the title ~~Python bindings article now partially works on win~~ Make python bindings article partially work on win Nov 1, 2020

jima80525 reviewed Nov 1, 2020

View reviewed changes

removed *.pyx from clean

e6a6a70

Znunu added 3 commits November 6, 2020 16:35

Added readme

e07259e

clean uses python lib to delete

3740d6f

formatted by black

30800e1

jima80525 approved these changes Nov 8, 2020

View reviewed changes

Merge branch 'master' into master

c17f314

jima80525 merged commit a14a6fc into realpython:master Nov 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make python bindings article partially work on win #157

Make python bindings article partially work on win #157

Znunu commented Oct 18, 2020

jima80525 commented Oct 20, 2020

Znunu commented Oct 20, 2020 •

edited

Loading

Znunu commented Oct 29, 2020

jima80525 commented Oct 30, 2020

jima80525 left a comment

jima80525 Oct 31, 2020

jima80525 Oct 31, 2020

jima80525 Oct 31, 2020

jima80525 Oct 31, 2020

jima80525 Oct 31, 2020

Znunu Nov 1, 2020

jima80525 Nov 1, 2020

jima80525 Oct 31, 2020

Znunu Nov 1, 2020

jima80525 Nov 1, 2020

jima80525 Oct 31, 2020

Znunu Nov 1, 2020

jima80525 Nov 1, 2020

jima80525 Oct 31, 2020

dbader Nov 4, 2020

jima80525 Nov 5, 2020

jima80525 Oct 31, 2020

Znunu commented Nov 1, 2020

jima80525 commented Nov 1, 2020

jima80525 commented Nov 1, 2020 •

edited

Loading

jima80525 Nov 1, 2020

jima80525 commented Nov 1, 2020

Znunu commented Nov 2, 2020

jima80525 commented Nov 2, 2020

jima80525 commented Nov 4, 2020

jima80525 commented Nov 5, 2020

Znunu commented Nov 6, 2020

jima80525 left a comment

jima80525 commented Nov 8, 2020

		@@ -1,4 +1,5 @@
		#!/usr/bin/env python3
		# IDE might complain with "no module found" here, even when it exists



		@invoke.task(build_cmult)
		@invoke.task()

Make python bindings article partially work on win #157

Make python bindings article partially work on win #157

Conversation

Znunu commented Oct 18, 2020

jima80525 commented Oct 20, 2020

Znunu commented Oct 20, 2020 • edited Loading

Znunu commented Oct 29, 2020

jima80525 commented Oct 30, 2020

jima80525 left a 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

Choose a reason for hiding this comment

Znunu commented Nov 1, 2020

jima80525 commented Nov 1, 2020

jima80525 commented Nov 1, 2020 • edited Loading

Choose a reason for hiding this comment

jima80525 commented Nov 1, 2020

Znunu commented Nov 2, 2020

jima80525 commented Nov 2, 2020

jima80525 commented Nov 4, 2020

jima80525 commented Nov 5, 2020

Znunu commented Nov 6, 2020

jima80525 left a comment

Choose a reason for hiding this comment

jima80525 commented Nov 8, 2020

Znunu commented Oct 20, 2020 •

edited

Loading

jima80525 commented Nov 1, 2020 •

edited

Loading