Problems implementing nimporter

juancarlospaco commented 3 years ago

I am trying to migrate https://github.com/juancarlospaco/faster-than-requests
The project is older than Nimporter and I didnt have the time before. I need to build for Windows, Mac, Linux.
I have made a PR were I deleted everything and only left the basic files, the .py and .nim files.

I have several errors but I dont know whats the problem, they all just exit on the GitHub Actions CI with:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
IndexError: list index out of range

It does not fully finish correctly, always fails on 1 of the 3 operating systems. I guess something is failing to build the files, but the error message is confusing. Maybe can you take a quick look at the branch and help me with the errors?:

:slightly_smiling_face:

Pebaz commented 3 years ago

Hello @juancarlospaco ! I will take a look and see what I can find out.

Pebaz commented 3 years ago

@juancarlospaco Is there any way you can move back to the example GitHub Actions workflow?

I was also tempted to try to combine all 3 platforms into a single workflow but ran into issues. I determined it would be easier to see the differences if they were kept apart (Windows, MacOS, Linux).

The index error is due to this line where the wheel file name is obtained. Since that line is failing it suggest that there is an issue with setup.py possibly.

Also, since the build.yml workflow is just to see if the software builds correctly and not upload to PyPi, I would remove this line and this line because those are used to upload to PyPi.

juancarlospaco commented 3 years ago

since the build.yml workflow is just to see if the software builds correctly and not upload to PyPi, I would remove this line and this line because those are used to upload to PyPi.

Yes and no, I mean because I know these lines are not doing anything, but I want it to upload to PyPI when it works Ok.

move back to the example GitHub Actions workflow

Ok :+1:

Pebaz commented 3 years ago

Cool! Let me know if I can help further! :)

If you happen to get it up and running let me know and I'll get this issue closed out.

juancarlospaco commented 3 years ago

Traceback (most recent call last):
  File "<string>", line 1, in <module>
IndexError: list index out of range

Now with the template workflow:

https://github.com/juancarlospaco/faster-than-requests/pull/133/checks?check_run_id=1865976103#step:7:1436

...and there it goes

Pebaz commented 3 years ago

I think I found something: https://github.com/juancarlospaco/faster-than-requests/runs/1865976125?check_suite_focus=true#step:4:7

It looks like the allenevans/set-env@v2.0.0 workflow is not working as expected. As far as I can tell, I think you can just use the "env" command like here although I could be wrong.

Also, I'm not sure if this is impacting anything, but the build artifact is coming out as: UNKNOWN-0.0.0-cp39-cp39-linux_x86_64.whl because the setup.py doesn't have a version or name. It might be worth adding those fields and seeing what happens.

juancarlospaco commented 3 years ago

Hit a bug on the setup nim action https://github.com/jiro4989/setup-nim-action/issues/40#issue-804924621

Pebaz commented 3 years ago

Ah interesting! Keep me posted and let me know if I can help!

juancarlospaco commented 3 years ago

Theres a way to ship the .nim files and make nimporter compile them on import instead of wheels or C files ?.

It can depend on https://pypi.org/project/choosenim-install to automatically install Nim on the PC.

Pebaz commented 3 years ago

Yes this is definitely possible but goes against the intended purpose of Nimporter. The goal was to be 100% transparent that Nim is involved from the side of the end user.

Initially, I built Nimporter to bundle C files but the Nim compiler generates specific C files per target. Distributing wheels therefore became the only way to transparently install Nim dependencies for Python.

Did the fix specified by jiro4989 solve the issue? It looks like the current version is 1.3.0.

juancarlospaco commented 3 years ago

Still the same error https://github.com/juancarlospaco/faster-than-requests/commit/ab50c5b6bf2e9c06933e546ec9ee9d3fc56d32a4

Pebaz commented 3 years ago

It looks like my username is inside the workflow lol: https://github.com/juancarlospaco/faster-than-requests/runs/1866497757?check_suite_focus=true#step:6:3

Also, can you edit one of the builds to ls dist and ls . to see what's in there? The command that is failing for Linux is that it cannot find the generated wheel.

juancarlospaco commented 3 years ago

If you PIP install https://github.com/juancarlospaco/choosenim_install#use It will install Nim automatically, so it will be just like pip install cython

Thats why I am interested in a way to just ship the .nim files.

Pebaz commented 3 years ago

I understand. This is a viable option if the installation of Nim is acceptable. Here is the old way of using Nimporter which should fit your use case:

setup(
    name='Foo',                     # Keep your existing arguments
    version='0.1.0',
    ...,
    package_data={'': ['*.nim']},   # Distribute Nim source files
    include_package_data=True,
    install_requires=['nimporter']  # Depends upon Nimporter
)

Pebaz commented 3 years ago

Oh one other thing is that if you use that method, the first time you run the software it will have to take the time to compile the Nim source. That was another reason I didn't opt for it but again this might not be a problem in many cases.

Pebaz commented 3 years ago

@juancarlospaco Were you able to use the source or binary distribution methods successfully?

juancarlospaco commented 3 years ago

@Pebaz Nope. While trying to just ship the .nim files I done:

setuptools.setup(
  package_data         = {'': ['*.nim']},   
  include_package_data = True,
)

Running python setup.py sdist.

Opening the ZIP, it has no .nim files inside, even if explicitly passing "*.nim" on setup.py and/or setup.cfg

temp-nimporter

Even if you force the file faster_than_requests.nim inside the ZIP manually, it does not copy the .nim file when installing the ZIP.

Therefore the instructions for source distributions on the readme are not really valid.

I know that it suppose to ship anything that you pass as argument on setup( ), but in reality it is not.

You can check the changes on this branch, ignore the CI, the point is that python setup.py sdist does not ship .nim files, you can check:

https://github.com/juancarlospaco/faster-than-requests/pull/138/files

Deleting the setup.cfg does not change anything.

:crying_cat_face:

juancarlospaco commented 3 years ago

Any ideas?, something to try?. :neutral_face:

Pebaz commented 3 years ago

I will look into this today and see what I can find out.

Pebaz commented 3 years ago

This is an interesting problem. I was able to successfully bundle Nim files into a source distribution by simply removing the include_package_data=True line from setup.py. No idea why that would work but try it out and let me know if it works for you and I'll update the source distribution instructions in the readme:

setup(
    ...,                            # Keep your existing arguments
    package_data={'': ['*.nim']},   # Distribute Nim source files
    # include_package_data=True, <- Remove just this line
    install_requires=[
        'nimporter',  # Must depend on Nimporter
        'choosenim_install'  # Optional. Auto-installs Nim compiler
    ]
)

juancarlospaco commented 3 years ago

I will try it!; But it copies the .nim files when you install that package ?.

Pebaz commented 3 years ago

Yes the test package I created had all the Nim files it needed. I was able to successfully import and use it! :)

Pebaz commented 3 years ago

One more thing: if you're using "nim.cfg" files, the *.nim line should be changed to read:

package_data={'': ['*.nim*']},  # Added an ending "*" so that "*nim.cfg" files are added also

juancarlospaco commented 3 years ago

Yes the test package I created had all the Nim files it needed. I was able to successfully import and use it! :)

Do you have that example somewhere?, if you can upload it to the examples on the repo or link it or something.

I can make the package with the *.nim using the setup.py, but when I install, it installs everything except the *.nim, I searched the folders recursively and the *.nim are not there.

juancarlospaco commented 3 years ago

I went with just choosenim_install and nimporter for now. Thanks. :slightly_smiling_face:

Pebaz commented 3 years ago

@juancarlospaco I did a proof of concept refactor on your branch here

It is currently not operational as I don't have your insight into it's internals.

However, it illustrates how projects using Nimporter should be structured (using a Python package rather than a src folder).

It's like 90% working, let me know if this helps or if I can assist further! :)

juancarlospaco commented 3 years ago

https://pypi.org/project/faster-than-requests/21.03.03/#files

Thanks for all the help, it is working now. :smiley: :+1:

Pebaz commented 3 years ago

Fantastic, glad this worked out!

Pebaz / nimporter

Problems implementing nimporter #39