Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add recipe for ckzg #26068

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Add recipe for ckzg #26068

wants to merge 17 commits into from

Conversation

step21
Copy link
Contributor

@step21 step21 commented Apr 17, 2024

Checklist

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

@conda-forge-webservices
Copy link

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.

@zklaus
Copy link
Contributor

zklaus commented May 7, 2024

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.

@step21
Copy link
Contributor Author

step21 commented May 7, 2024

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.

@conda-forge-webservices
Copy link

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:

@conda-forge-webservices
Copy link

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.

recipes/ckzg/meta.yaml Outdated Show resolved Hide resolved
recipes/ckzg/meta.yaml Outdated Show resolved Hide resolved
@step21
Copy link
Contributor Author

step21 commented Jun 19, 2024

@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.

@hmaarrfk
Copy link
Contributor

what if you removed clang alltogether?

@step21
Copy link
Contributor Author

step21 commented Jun 20, 2024

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)

@isuruf
Copy link
Member

isuruf commented Jun 24, 2024

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

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

@step21
Copy link
Contributor Author

step21 commented Jun 24, 2024

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

@jakirkham
Copy link
Member

Maybe disable Windows and then we can review/merge?

The Windows portion could be revisited in the feedstock

@step21
Copy link
Contributor Author

step21 commented Jun 25, 2024

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
Copy link
Contributor Author

step21 commented Jun 25, 2024

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

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Florian! 🙏

Added a couple minor suggestions below

recipes/ckzg/meta.yaml Outdated Show resolved Hide resolved
recipes/ckzg/build.patch Outdated Show resolved Hide resolved
recipes/ckzg/meta.yaml Outdated Show resolved Hide resolved
recipes/ckzg/meta.yaml Outdated Show resolved Hide resolved
step21 and others added 4 commits June 26, 2024 01:16
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
recipes/ckzg/meta.yaml Outdated Show resolved Hide resolved
@step21
Copy link
Contributor Author

step21 commented Jun 27, 2024

Hey, @jakirkham without any patch, we are now again at the juncture of this Makefile I referred to here: #26068 (comment)
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?

@MementoRC
Copy link
Contributor

@step21 After a few days looking into it, I proposed this: #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.

@step21
Copy link
Contributor Author

step21 commented Aug 28, 2024

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.

@MementoRC
Copy link
Contributor

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
Copy link
Contributor

Similar functionality with separated blst/ckzg build to address the upstream package assumptions on compiler: #27490

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants