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

Exception raised during wheel build. #571

Closed
KerstinKeller opened this issue Dec 13, 2023 · 11 comments · Fixed by #601
Closed

Exception raised during wheel build. #571

KerstinKeller opened this issue Dec 13, 2023 · 11 comments · Fixed by #601

Comments

@KerstinKeller
Copy link

We're trying to use scikit-build-core to build eCAL wheels for Python.
Our previous build workflow involved first building Python extensions with CMake and later packaging them up, which doesn't result in an sdist and we would like to move away from it.

eCAL is a C++ middleware with a Python binding and multiple native dependencies.
It's not trivial to build, and therefore also not trivial to use scikit-build-core for it.

Our current attempt to build can be found here https://github.com/eclipse-ecal/ecal/blob/feature/scikit-core-build/pyproject.toml, and I am trying to build locally with python -m build ..

The sdist is created fine, but I get the following error trying to build the wheel:

* Installing packages in isolated environment... (pathspec, pyproject_metadata)
* Building wheel...
*** scikit-build-core 0.7.0 using CMake 3.22.1 (wheel)
Traceback (most recent call last):
  File "C:\Users\uidv8497\AppData\Local\Programs\Python\Python39\lib\site-packages\pyproject_hooks\_in_process\_in_process.py", line 353, in <module>
    main()
  File "C:\Users\uidv8497\AppData\Local\Programs\Python\Python39\lib\site-packages\pyproject_hooks\_in_process\_in_process.py", line 335, in main
    json_out['return_val'] = hook(**hook_input['kwargs'])
  File "C:\Users\uidv8497\AppData\Local\Programs\Python\Python39\lib\site-packages\pyproject_hooks\_in_process\_in_process.py", line 251, in build_wheel
    return _build_backend().build_wheel(wheel_directory, config_settings,
  File "C:\Users\uidv8497\AppData\Local\Temp\build-env-ipb8z2t0\lib\site-packages\scikit_build_core\build\__init__.py", line 31, in build_wheel
    return _build_wheel_impl(
  File "C:\Users\uidv8497\AppData\Local\Temp\build-env-ipb8z2t0\lib\site-packages\scikit_build_core\build\wheel.py", line 216, in _build_wheel_impl
    license_files = {
  File "C:\Users\uidv8497\AppData\Local\Temp\build-env-ipb8z2t0\lib\site-packages\scikit_build_core\build\wheel.py", line 217, in <dictcomp>
    x: x.read_bytes()
  File "C:\Users\uidv8497\AppData\Local\Programs\Python\Python39\lib\pathlib.py", line 1259, in read_bytes
    with self.open(mode='rb') as f:
  File "C:\Users\uidv8497\AppData\Local\Programs\Python\Python39\lib\pathlib.py", line 1252, in open
    return io.open(self, mode, buffering, encoding, errors, newline,
  File "C:\Users\uidv8497\AppData\Local\Programs\Python\Python39\lib\pathlib.py", line 1120, in _opener
    return self._accessor.open(self, flags, mode)
PermissionError: [Errno 13] Permission denied: 'licenses'

Do you have any idea what could be going wrong?
Do you have some links to some more complicated projects using scikit-build-core than the trivial examples which could be used as a reference?

@LecrisUT
Copy link
Collaborator

I think the issue is that project.license is not sufficient, and instead tool.scikit-build.wheel.license-files needs to be set (the default values for that do not pick up LICENSE.txt, and instead it picks up the folder licenses (since windows is case-insensitive)). Try setting tool.scikit-build.wheel.license-files explicitly.

@henryiii shouldn't the project.license be used instead when it contains a file reference? Probably issue is when the project has multiple licenses, which apparently is the case for this project being built.

@henryiii
Copy link
Collaborator

Yes, it probably should be. I was rather hoping for PEP 639 to get accepted at some point, but we should support the old way for now. I was looking at Flit a while back and noticed it does mark the file to be included in the SDist based on the value of this field, so it's not completely unused. Won't really help too much here if you actually wanted all the licenses, though, since this one has a lot (which is why it's tripping up on the directory).

As already mentioned, tool.scikit-build.wheel.license-files should fix it. We should probably improve this when licenses is a directory instead of a file. And make sure it supports globbing!

@henryiii
Copy link
Collaborator

For links, we are going to be compiling a list in the very near future, but you can always look though https://github.com/search?type=code&q=path%3Apyproject.toml+scikit_build_core+NOT+is%3Afork.

@henryiii
Copy link
Collaborator

And let us know if anything is needed! Happy to help support.

@henryiii
Copy link
Collaborator

henryiii commented Dec 15, 2023

At today's Scikit-Build Community Meeting, it was pointed out by @vyasr that RAPIDS has moved to scikit-build-core. Example: https://github.com/rapidsai/cudf/blob/cbcfa678392cec07c33f57090a793c3631d19c7e/python/cudf/pyproject.toml#L4

@henryiii
Copy link
Collaborator

(though, @vyasr, cmake and ninja should not be listed there)

@vyasr
Copy link
Contributor

vyasr commented Dec 15, 2023

@henryiii we decided to go against the scikit-build-core recommendation here. On one hand, RAPIDS does not currently run on any platforms for which cmake wheels would not be available, so we don't have the downside risk of failure to install cmake breaking a build. On the other hand, RAPIDS tends to be running with very new CMake versions because we're fairly active in contributing features and bug fixes back to CMake (especially around CUDA support as you may imagine), but this also makes us extra vulnerable to CMake bugs because we're often using newer features. As a result, a lower bound constraint isn't always sufficient; we've needed upper bounds or to exclude specific versions of CMake on more than one occasion. As a result, we decided that the importance of having more complete control over the CMake version used outweighed the value of being able to use a system CMake on platforms where the wheels didn't exist.

For ninja that's less important, we just included it for completeness since we're already including cmake.

@henryiii
Copy link
Collaborator

henryiii commented Dec 15, 2023

I wonder if it would make sense to replace

[tool.scikit-build]
cmake.minimum-version = "3.15"

with

[tool.scikit-build]
cmake.version = ">=3.15"

I think we chose the former because we might be able to do something with cmake_required_version, but we haven't yet. Hmm, just a thought.

@vyasr
Copy link
Contributor

vyasr commented Dec 15, 2023

If cmake.minimum_version were replaced by a cmake.version that supported a full constraint, we would be willing to remove it from build-system.requires.

@LecrisUT
Copy link
Collaborator

LecrisUT commented Dec 15, 2023

How about having skbuild-core support pythonic version matching in tool.scikit-build.cmake.version? Given @vyasr explanation it seems it would be useful to have the cmake requirement version outside of build-requirments so that developers can more easily test with their local CMake version.

This is assuming that the cmake.version parsing is handled when checking the local and pypi version of CMake.

Edit: I'm slow at typing. Seems we all have the same thought

@vyasr
Copy link
Contributor

vyasr commented Dec 21, 2023

Raised #583

henryiii added a commit that referenced this issue Jan 3, 2024
This would help
#571 not have
shown an error (but in that case, multiple licenses is actually desired,
and the custom `tool.scikit-build` setting should be used until PEP 639
goes somewhere).

---------

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants