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

chore(build): move to scikitbuild-core #312

Closed
wants to merge 17 commits into from

Conversation

mayeut
Copy link
Contributor

@mayeut mayeut commented Nov 5, 2022

@henryiii, just a draft to follow progress with scikitbuild-core. You're obviously well aware of what happens there but I wanted to give it a try already.

@mayeut mayeut force-pushed the scikit-build-core branch 2 times, most recently from f9ddf79 to a17ec66 Compare November 5, 2022 10:49
pyproject.toml Outdated

[project]
name = "cmake"
version = "0.0.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess dynamic versions are for the future ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty low priority for now, as it adds significant extra complication for very little benefit for now. Eventually will probably add support for reading from a regex & setuptools_scm.

"Topic :: Software Development :: Build Tools",
"Typing :: Typed"
]
requires-python = ">=3.7"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be a showstopper (for now) for this project ?

Copy link
Contributor

@henryiii henryiii Nov 5, 2022

Choose a reason for hiding this comment

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

scikit-build/scikit-build-core#39 is needed. It won't ever build from SDist on Python <3.7, but don't think that's a big deal at this point.

Ideally we can just set a configuration parameter to tell it we don't depend on Python, and it will produce py2.py3 wheels automatically. Maybe it could even be smart enough to check the output binaries, and report if they depend on Python (normal or ABI3), or don't? (It's important to answer that question for the extensionlib work too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scikit-build/scikit-build-core#39 is needed

It would be nice to have it but it's not needed. It would allow to get rid of all the specific repair scripts in the repo.

It won't ever build from SDist on Python <3.7, but don't think that's a big deal at this point.

I'm a bit more hesitant on "runs everywhere", "need >= 3.7 to build from sdist". In the end, it will be ok but might be a bit too soon ?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we build from SDist, I think it's already much slower, more likely to fail, etc - it's really supposed to be a wheel distribution. So not supporting building from SDist on <=3.6 doesn't seem too bad. And a user can always either use Python 3.7+ to build a wheel then install it on 2.7-3.6, or use an older version.

@mayeut
Copy link
Contributor Author

mayeut commented Nov 5, 2022

@ henryiii, you might want to have a look at the sdist failure. From my long distance, it seems it could be a bug in pip/pep517 rather than one in scikitbuild-core but I may be mistaken.

EDIT: this was due to scikit-build/ninja-python-distributions#157

@henryiii
Copy link
Contributor

henryiii commented Nov 5, 2022

I'll investigate more, though a quick thought: build's output is better (it lets you know the packages it's trying to install each step), maybe that could be swapped out for pip? Maybe even build the wheel then use installer to install it (haven't tried it yet). It's possible it's confused because a build dependency is the package you are also trying to build. No, this is on ninja for the cmake package.

@henryiii
Copy link
Contributor

henryiii commented Nov 5, 2022

I'm providing a way to set logging level in the next version (scikit-build/scikit-build-core#40), I can add some extra logging to see why it can't import Ninja. Maybe it's not adding ninja even though it's not present?

@mayeut
Copy link
Contributor Author

mayeut commented Nov 6, 2022

Thanks @henryiii,

so the whole mess with sdist was linked to the broken ninja package in the end.

@mayeut
Copy link
Contributor Author

mayeut commented Nov 6, 2022

the macOS wheel has a macosx_11_0_x86_64 tag instead of macosx_10_10_x86_64. I'll try to investigate this.

CMakeLists.txt Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@henryiii
Copy link
Contributor

FYI, @agoose77, I think the reproducible SDists feature is breaking this CMake test:

CMake Error at /project/CMake-src/Tests/ssage):
  cmake_depends - FAILED:

  stdout does not match that expected.

  Command was:

   command> "/tmp/tmp2x2jq55h/build/-E" "cmake_depends" "Unix Makefiles" "/e/CommandLine/cmake_depends" "/project/CMake-cmake_depends" "/tmp/tmp2x2jq55h/build/ke/CommandLine/cmake_depends-build" "/tmp/build/Tests/RunCMake/CommandLine/x2jq55h/build/CMakeProject-build/Tests/nds-build/CMakeFiles/DepTarget.dir/

  Expected stdout to match:

   expect-out> ^Scanning dependencies of 

  Actual stdout:

   actual-out> Dependee "/tmp/tmp2x2jq55h/RunCMake/CommandLine/cmake_depends-build/Info.cmake" is newer than depender "/tmp/build/Tests/RunCMake/CommandLine/DepTarget.dir/depend.internal".
   actual-out> Dependee "/tmp/tmp2x2jq55h/RunCMake/CommandLine/cmake_depends-build/ation.cmake" is newer than depender "/tmp/build/Tests/RunCMake/CommandLine/DepTarget.dir/depend.internal".
   actual-out> Scanning dependencies of 

  Actual stderr:

   actual-err>

Call Stack (most recent call first):
  /project/CMake-src/Tests/RunCMake/
  /project/CMake-src/Tests/RunCMake/944 (run_cmake_command)
  /project/CMake-src/Tests/RunCMake/951 (run_cmake_depends)

@agoose77
Copy link

@henryiii it looks like it might not be the reproducibility patch; the CI still fails in the same place as far as I can see.

@henryiii
Copy link
Contributor

henryiii commented Nov 15, 2022

The commit right before the repro patch works. Tomorrow I'll try the commit after.

Not totally sure why changing the SDist would break the wheel tests...

Edit, no, wait, I think a few failed. I'll check tomorrow.

e4ed476a77b13d36379ed7e2ba52496cfcd00c4b (61) Fails.
4f5bb9494c365cebf54391aa36a2f898bfee3536 (57) Fails.
f8d7a3727e1515e2946b3d1fab323ade087ae9a5 (56) Works.
75754d0305a215c81cda90e159addad8f82cb323 Works.
2bbc1aa4307cb5e351156b831e5b6ec1f27589ae (53) Works. (still checking)

It was the make fallback. One test fails if it falls back on make.

mayeut and others added 6 commits November 16, 2022 21:08
Update pyproject.toml

Apply suggestions from code review

Update pyproject.toml

WIP: try branch of skbc

Update pyproject.toml

WIP: right before repro builds

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

Update pyproject.toml
pyproject.toml Outdated Show resolved Hide resolved
@henryiii henryiii closed this Nov 17, 2022
@henryiii henryiii reopened this Nov 17, 2022
@henryiii
Copy link
Contributor

cmake-3.25.0-py2.py3-none-macosx_10_10_universal2.macosx_11_0_universal2.macosx_10_10_x86_64.macosx_11_0_arm64.whl   43,993 kB

name looks right. :)

* master: (112 commits)
  docs: fix up docs (scikit-build#454)
  chore(deps): update pre-commit hooks (scikit-build#443)
  chore(deps): bump cmake from 3.28.1 to 3.28.3 (scikit-build#452)
  chore(deps): bump the actions group with 1 update (scikit-build#453)
  Update to CMake 3.28.3 (scikit-build#450)
  chore(ci): use 4 threads to compile on GHA (scikit-build#449)
  Update to OpenSSL 3.0.13 (scikit-build#448)
  chore(deps): bump the actions group with 1 update (scikit-build#447)
  chore(deps): update pre-commit hooks (scikit-build#441)
  ci: group dependabot updates (scikit-build#442)
  chore(deps): bump cmake from 3.27.9 to 3.28.1 (scikit-build#440)
  Update to CMake 3.28.1 (scikit-build#439)
  ci: support artifact v4 (scikit-build#438)
  chore(deps): bump actions/upload-artifact from 3 to 4
  chore(deps): bump actions/download-artifact from 3 to 4
  Update to CMake 3.28.0
  chore(deps): update pre-commit hooks
  chore(deps): bump actions/setup-python from 4 to 5 (scikit-build#431)
  chore(deps): bump cmake from 3.27.7 to 3.27.9 (scikit-build#430)
  Update to CMake 3.27.9 (scikit-build#429)
  ...
Test command is already specified in pyproject.toml
From scikit-build#312 (comment)

> If we build from SDist, I think it's already much slower, more likely
> to fail, etc - it's really supposed to be a wheel distribution. So not
> supporting building from SDist on <=3.6 doesn't seem too bad. And a user
> can always either use Python 3.7+ to build a wheel then install it on
> 2.7-3.6, or use an older version.
…ualenv

The use of the `virtualenv` pytest fixture become obsolete following
commit 280260d ("chore(ci): bump `test_sdist` python from 3.11 to 3.12
(scikit-build#423)", 2023-12-02)
@jcfr jcfr force-pushed the scikit-build-core branch 2 times, most recently from 518334b to ebedcde Compare February 15, 2024 19:06
@jcfr
Copy link
Contributor

jcfr commented Feb 15, 2024

@mayeut @henryiii This is now ready for review.

@jcfr jcfr marked this pull request as ready for review February 15, 2024 22:33
metadata.version.provider = "scikit_build_core.metadata.setuptools_scm"
ninja.make-fallback = false
sdist.include = ["src/cmake/_version.py"]
wheel.py-api = "py2.py3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not in line with requires-python = ">=3.7" and I think pip will not install the wheel from PyPI when running python 2.7/3.6.
Can/Shall we drop/update the requires-python ?
If we still support 2.7/3.6 then they shall still be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python 2.7 & 3.5 can't use the wheel produced. I'm checking what can be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened #455 depending on python<3.7 support requirement (for wheels).

@jcfr
Copy link
Contributor

jcfr commented Feb 25, 2024

Closing. Superseded by #455

@jcfr jcfr closed this Feb 25, 2024
@mayeut mayeut deleted the scikit-build-core branch March 2, 2024 11:47
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 this pull request may close these issues.

None yet

4 participants