staged-recipes icon indicating copy to clipboard operation
staged-recipes copied to clipboard

Add recipe for ckzg

Open step21 opened this issue 2 years ago • 22 comments

Checklist

  • [x] Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • [x] License file is packaged (see here for an example).
  • [x] Source is from official source.
  • [x] Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • [x] If static libraries are linked in, the license of the static library is packaged.
  • [x] Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • [x] Build number is 0.
  • [x] A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • [x] GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • [x] When in trouble, please check our knowledge base documentation before pinging a team.

step21 avatar Apr 17 '24 11:04 step21

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ckzg) and found it was in an excellent condition.

Note that upstream has in the meantime added a patch for the Windows issue at https://github.com/ethereum/c-kzg-4844/pull/419. You can turn that into a patch here, or skip Windows until a new release with the patch comes out.

zklaus avatar May 07 '24 07:05 zklaus

Note that upstream has in the meantime added a patch for the Windows issue at ethereum/c-kzg-4844#419. You can turn that into a patch here, or skip Windows until a new release with the patch comes out.

I saw this, because as you can see I was in that discussion with upstream, but there is no indication that that patch solves this particular windows issue. I had wanted to test it, but as I wrote had/have trouble getting a system able to test it.

step21 avatar May 07 '24 12:05 step21

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ckzg) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipes/ckzg:

  • This recipe is using a compiler, which now requires adding a build dependence on {{ stdlib("c") }} as well. For further details, please see https://github.com/conda-forge/conda-forge.github.io/issues/2102.

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ckzg) and found it was in an excellent condition.

@conda-forge/core please review/approve with the original build script or tell me how I can force/replace the build script to use only clang. The way @xhochy suggested did not work, thus I reverted the commits to a working build.

step21 avatar Jun 19 '24 21:06 step21

what if you removed clang alltogether?

hmaarrfk avatar Jun 19 '24 23:06 hmaarrfk

what if you removed clang alltogether?

I assume it would still clash with this Makefile https://github.com/ethereum/c-kzg-4844/blob/main/src/Makefile which has

# Cross-platform compilation settings.
ifeq ($(PLATFORM),Windows)
	CC = gcc
	CFLAGS += -D_CRT_SECURE_NO_WARNINGS
else
	CC = clang
	CFLAGS += -fPIC -Werror
endif

Windows also probably does not really use this file, as in setup.py there is

class CustomBuild(build_ext):
    def run(self):
        if system() == "Windows":
            try:
                check_call(["blst\\build.bat"])
            except Exception:
                pass
        check_call(["make", "-C", "src", "blst"])
        super().run()

I'm not saying these are great, but I assume I would have to patch them only use either clang or gcc. (and I do not know if there was a reason to only use either)

step21 avatar Jun 20 '24 15:06 step21

you'll have to patch setup.py to use something like

check_call(["make", "-C", "src", "blst", f"CC={os.environ['CC']}"])

isuruf avatar Jun 24 '24 05:06 isuruf

This unfortunately fails on Win, which still uses cl.exe. Any ideas? (I also patched out the use of the windows bat file, as it didn't use either gcc or clang and I'm not sure what kind of magic it is doing. https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=961378&view=logs&jobId=240f1fee-52bc-5498-a14a-8361bde76ba0&j=240f1fee-52bc-5498-a14a-8361bde76ba0&t=7c0f8eae-6d6f-51bf-636f-73a1a7fb1bc4

step21 avatar Jun 24 '24 14:06 step21

Maybe disable Windows and then we can review/merge?

The Windows portion could be revisited in the feedstock

jakirkham avatar Jun 25 '24 07:06 jakirkham

Maybe disable Windows and then we can review/merge?

The Windows portion could be revisited in the feedstock

Ok good idea. Windows build is skipped now.

step21 avatar Jun 25 '24 15:06 step21

This is ready for review, @conda-forge/help-python

step21 avatar Jun 25 '24 21:06 step21

Hey, @jakirkham without any patch, we are now again at the juncture of this Makefile I referred to here: https://github.com/conda-forge/staged-recipes/pull/26068#issuecomment-2181023138 This forces clang for linux and osx. On OSX that works, because it is installed by default. The main question is - how to force it to use only one compiler? In the pursuit of first merging, then fixing the feedstock, should I add clang back, so that only clang is used?

step21 avatar Jun 27 '24 16:06 step21

@step21 After a few days looking into it, I proposed this: https://github.com/conda-forge/staged-recipes/pull/27412, which should build Windows using gcc 5, separate blst as its own package (with the py-binding being worked in a separate recipe that still has a few snags on linux) Note that it is still work in progress as I need to fine-tune windows/osx by going through CI.

MementoRC avatar Aug 27 '24 23:08 MementoRC

Awesome, thanks for your work. Looks great from what I can tell. I don't know yet if I can take a look at the remaining build failures.

step21 avatar Aug 28 '24 13:08 step21

Cool. I am still working on it. I want to enforce linking to the versioned dynamic library and it's a bit trickier. It feels more reliable, but maybe overkill. As for windows, it's my Achilles heel, I had it working but must have changed something during the many refactoring. I'll ping you when I get it to pass

MementoRC avatar Aug 28 '24 14:08 MementoRC

Similar functionality with separated blst/ckzg build to address the upstream package assumptions on compiler: https://github.com/conda-forge/staged-recipes/pull/27490

MementoRC avatar Sep 05 '24 15:09 MementoRC

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ckzg/meta.yaml) and found it was in an excellent condition. I do have some suggestions for making it better though...

For recipes/ckzg/meta.yaml:

  • No valid build backend found for Python recipe for package ckzg using pip. Python recipes using pip need to explicitly specify a build backend in the host section. If your recipe has built with only pip in the host section in the past, you likely should add setuptools to the host section of your recipe.

conda-forge-admin avatar Oct 14 '24 11:10 conda-forge-admin

thanks for your comments. TBH I don't really care which PR we continue. Also added you as maintainer here. Though your has a lot more changes, I only patched out the build.bat so far, so I assume I would also have to carry those over.

step21 avatar Oct 14 '24 15:10 step21

builds! all pkgs are using clang. Only thing is that there is a deprecation warning for setup.py, but I would ignore it for now.

step21 avatar Oct 14 '24 16:10 step21

Looks good to me!

wolfv avatar Oct 22 '24 11:10 wolfv

Thanks @jakirkham for the suggestions, should all be applied now.

step21 avatar Oct 26 '24 08:10 step21

Are we waiting for one more review now?

step21 avatar Nov 06 '24 16:11 step21

Right there were a bunch of folks looking at this. So was waiting to see if anyone else still had more thoughts

jakirkham avatar Nov 06 '24 23:11 jakirkham

Are we still waiting?

step21 avatar Nov 11 '24 11:11 step21

thanks!

step21 avatar Nov 16 '24 19:11 step21