Adding source distribution

[step21]

Hey, this should add source packages to the python packages. I don't really know of a good way to test github actions locally, but if you do, would be happy to do so. This is basically why it is still labeled WIP.

• I used a separate build step, because from what I read, the cibuildwheel thing you use does not support source packages.

[jtraglia]

Hey @step21 thanks so much for this! In general it looks great, I'm a little concerned about a path problem. I'm not sure it will actually get published with the wheels. I'll need to do some investigation. And yes, testing locally is difficult. I'm a bit busy with other things at the moment, but I'll try to get around to properly reviewing this soon.

[step21]

In my investigation, setup.py etc create wheels and sdist all in the same repository. And in examples like sklearn, it seems they also use the same github action as you do for actual upload. So I made sure to upload artifacts to the same name, and ideally the path should not be an issue. (But I didn't verify this yet)
Anyway, no worries, happy to answer what I can once you get around to it.

[jtraglia]

Hey again. So the source distribution only contains code in the Python bindings directory. It doesn't contain src/c_kzg_4844.* and the blst library which we need. So we wouldn't actually be able to build ckzg from this.

$ unp ckzg-1.0.0.tar.gz
x ckzg-1.0.0/
x ckzg-1.0.0/PKG-INFO
x ckzg-1.0.0/README.md
x ckzg-1.0.0/ckzg.c
x ckzg-1.0.0/ckzg.egg-info/
x ckzg-1.0.0/ckzg.egg-info/PKG-INFO
x ckzg-1.0.0/ckzg.egg-info/SOURCES.txt
x ckzg-1.0.0/ckzg.egg-info/dependency_links.txt
x ckzg-1.0.0/ckzg.egg-info/top_level.txt
x ckzg-1.0.0/setup.cfg
x ckzg-1.0.0/setup.py

I'm not sure what the proper solution to this is. Maybe moving setup.py into the projects root directory. I'll need to think about it some. It might be easiest to just not provide a source distribution. Do we not publish a wheel for your system?

Edit: moving setup.py to the project's root wouldn't really solve the problem either. Because we need to build the blst library in a separate step before building the python wheel. I don't believe setuptools provides the ability to build a separate library first.

[step21]

Thanks for taking a stab. I think you should be able to specify additional files for the source, but yeah, moving setup.py to the main directory could work too.
The reason for needing a source distribution is because I maintain the conda-forge packages for web3.py and most related ones. Recently c-kzg was added as a dependency, but because it is not an existing package, it is not available. Conda-forge strives to provide clean and predictable packages, which means for example that vendored dependecies are only accepted in very limited circumstances. Ideally, there would be a source distribution from pypi. I will also take another look.
More complex packages usually have more complex build system anyway and stuff like shell scripts for at least some part. So what should work is either a bash script itself that then as second stage invokes setup.py or setup.py that calls a bash script or compile commnand such as described here: https://stackoverflow.com/questions/17887905/python-setup-py-to-run-shell-script

[step21]

Hey, I added a MANIFEST.in file to include the relevant files. (it might be more files than needed, but the archive is still super small) I would also be possible to add this in setup.py directly, but I thought this way would be cleaner.
I am a bit worried as it will now extract files to ../../blst etc., which means it could put stuff where noboday expects it (or it won't have permission) I asked some people for advice on this, but at least all file should be there.

[jtraglia]

Hey @step21, the manifest file is a good idea! I think this might be the solution. Yes, I'm not a fan of it extracting to parent directories, but we can fix that by moving setup.py into the project's root. I will play around with this a bit and might push a few commits.

[jtraglia]

@step21 how important is Windows to you?

[jtraglia]

So it's working on Windows, but it's a bit weird. Building blst.lib with MSVC first seems to fix things. Sorry about all of the commits, I don't have a Windows system to test on, so I rely on CI to test things. I believe this will work, so I'm going to merge this and manually publish a v1.0.1 release for the Python package. This didn't get pushed last time. Please let me know if this is acceptable to you and if so I will close the issue. If not, we'll fix whatever it is.

[jtraglia]

And here's confirmation that the source distribution is there:

[step21]

awesome, thanks a lot!

[step21]

FWIW, I also now have an annoying windows error. (just FYI)

[jtraglia]

FWIW, I also now have an annoying windows error. (just FYI)

What is the error? We might be able to fix it.

[step21]

FWIW, I also now have an annoying windows error. (just FYI)

What is the error? We might be able to fix it.

I'm not sure it's your fault / that of c-kzg. :) But in case you want to take a look: Build log: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=917127&view=logs&jobId=240f1fee-52bc-5498-a14a-8361bde76ba0&j=240f1fee-52bc-5498-a14a-8361bde76ba0&t=7c0f8eae-6d6f-51bf-636f-73a1a7fb1bc4

  File "C:\bld\ckzg_1713367077542\_h_env\lib\site-packages\setuptools\_distutils\util.py", line 132, in convert_path
      raise ValueError("path '%s' cannot be absolute" % pathname)
  ValueError: path '/home/runner/work/c-kzg-4844/c-kzg-4844/bindings/python/ckzg.c' cannot be absolute
  error: subprocess-exited-with-error

Further up it seems to take this path from "SOURCES.txt" but not sure where that is generated.

[jtraglia]

Hmm interesting. I suspect it's because absolute paths are used in setup.py.

I could make these files (and the other usages) relative. What do you think?

c-kzg-4844/setup.py

Line 40 in a874006

sources=[f("bindings/python/ckzg.c"), f("src/c_kzg_4844.c")],

Where, f is defined as:

c-kzg-4844/setup.py

Lines 11 to 12 in a874006

	def f(path_str):
	return str(this_dir / path_str)

And this_dir is:

c-kzg-4844/setup.py

Line 7 in a874006

this_dir = Path(__file__).parent

[step21]

Ah that makes sense. Well it it's not too much trouble. Sorry for Windows being weird, personally I wouldn't care but some ppl use it :) If you want me to test locally, I have a Windows machine at another desk and could test that later.

[jtraglia]

Not too much trouble! I'd like for it to work properly on Windows. If you could test out the changes from that new PR, that would be great. Afterwards, we can merge it. There's no rush though.