Skip to content

Modern Python packaging #147

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

Closed
wants to merge 6 commits into from
Closed

Conversation

hongquan
Copy link

Motivation

I have a Django web project, which targets Linux server but some team members use MacOS for development. We use uv as Python package manager. In the "dependency resolution" phase, uv tried to find systemd-python metadata but couldn't get what it wanted from the setup.py file. It had to build systemd-python wheel to get the metadata, then failed on MacOS machines.

What this PR does

Adopt new pyproject.toml file, in place of setup.py. For building C code, I adopt Meson, which has meson-python as a build backend so that pip or any other modern Python package manager can know how to build the C-based extensions.

Because this new build process is done in an isolated environment and leave no *.so files, the Makefile is also updated to work with new build way.

Copy link
Contributor

@behrmann behrmann left a comment

Choose a reason for hiding this comment

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

This is a very nice change! I gave it a quick go and it builds and works nicely.

@hongquan
Copy link
Author

Converted Makefile to Meson run targets. It requires me to introduce uv, to wrap the commands with uv run, to ensure the tools are installed (by uv) before running. But it then breaks the building docs in GitHub Action. I'm trying to fix...

@hongquan hongquan force-pushed the feature/meson-build branch from fcaab8e to af80340 Compare June 26, 2025 09:04
@hongquan
Copy link
Author

It requires me to introduce uv, to wrap the commands with uv run

No longer wrap, because it creates circular reference: If we build the project with uv build, uv will run meson indirectly under the hood (via meson-python build backend). If we wrap Meson target with uv, uv will then under meson (because the task is run as meson compile -C _build task).

@hongquan
Copy link
Author

@behrmann I moved Makefile jobs to Meson "run target". It turns out to be not convenient for developers, because:

  • Meson check for availability of the binaries in target command, like rsync, gpg even when we just want to build the Python extensions.

    run_target(
            'sign',
            command: ['gpg', '--detach-sign', '-a', archive],
    )
  • Meson always run the "taget" inside the _build folder, which is the same folder we use for building Python extension. It means that the meson we use for running targets must be the same version of the one invoked by meson-python build backend. It leads me to declare meson in both two places, like this:

    [build-system]
    requires = ["meson-python", "ninja", "meson"]  #  This is to be invoked by mesonpy build backend
    build-backend = "mesonpy"
    
    [dependency-groups]
    build = [
      "meson>=1.8.2",   #  This is for us to run other targets in meson.build
      "twine>=4.0.2",
    ]

@behrmann
Copy link
Contributor

Sorry for the slow turnaround, but busy with other stuff

Meson check for availability of the binaries in target command, like rsync, gpg even when we just want to build the Python extensions.

I've not run into this myself before, but looking at the systemd meson.build I think the workaround is wrapping the called binary in a call to end

env = find_program('env')
# …
run_target(
    'ctags',
    command : [env, 'ctags', '--tag-relative=never', '-o', '@0@/tags'.format(meson.project_source_root())] + all_files)
)

Meson always run the "taget" inside the _build folder, which is the same folder we use for building Python extension. It means that the meson we use for running targets must be the same version of the one invoked by meson-python build backend. It leads me to declare meson in both two places, like this:

I'm not quite sure I understand this, but I assume you are saying that you include meson in a build dependency group simply to signify that it is needed to run the run_targets? That sounds reasonable to me.

@hongquan
Copy link
Author

@behrmann That "wrapping with env" is a funny trick. If not hitting this Meson behavior, I will not understand why people have to do that. I think the migration is Ok now, although the experience of running "build docs", "test" with Meson will be surprising for Python dev coming from other projects.

Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. I really like the idea of using meson / python-meson and modernizing the build infrastructure. Nevertheless, this series cannot be merged as is, because it does a lot of unrelated and unexplained changes. I see that you did a lot exploratory work, which is great, but in the end we need "finished" patches that do one thing at a time.

Some things to avoid:

  • having whitespace formatting changes in the middle of the series. If you're adding new files, then add the format definitions first, and then add new files second.
  • lock files. Committing lock files to git doesn't make sense.
  • doing too many things in a single commit and not having any descriptions on the commits.
  • adding some lines and then doing unexpected changes to them later. E.g. in one of the first patches you add some meson config, and then change the order in the file in a later patch. Or you introduce a command with etags first, and change that to ctags later. This creates noise in the diff and confusion in the reviewer.

As mentioned below, I think the attempt to provide tight integration with uv makes things unnecessarily complicated. Let's make things work with meson first, so that the project can be nicely built locally. We can than work on integration with wrappers like uv separately.

on:
pull_request:
push:
branches:
- main
- feature/meson-build
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Author

@hongquan hongquan Jun 28, 2025

Choose a reason for hiding this comment

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

This is just to make GH Action run on my fork repo when I push the code.
The line will be deleted when the PR is approved.

- os: "ubuntu-22.04"
python: "3.13"
- os: "ubuntu-24.04"
python: "3.7"
Copy link
Member

Choose a reason for hiding this comment

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

3.7 is very old at this point. I guess we could drop that. But OTOH, the code will work fine with those old versions, so it should be fine to keep testing and supporting those old versions until there's some particular reason to raise the minimum.

@@ -8,6 +8,7 @@ on:
push:
branches:
- main
- feature/meson-build
Copy link
Member

Choose a reason for hiding this comment

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

??

on:
pull_request:
push:
branches:
- main
- feature/meson-build
Copy link
Member

Choose a reason for hiding this comment

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

??

Comment on lines +10 to 11
_build
build
Copy link
Member

Choose a reason for hiding this comment

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

I see both build and _build here. We normally use build for the build directory. I'd prefer not to introduce _build.

meson.build Outdated
# Run target to generate TAGS file for Emacs
run_target(
'etags',
command: ['etags', '-R', '@SOURCE_ROOT@/systemd'],
Copy link
Member

Choose a reason for hiding this comment

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

The docs don't make this super clear, but I think @SOURCE_ROOT@ shouldn't be used. The docs do say that meson.source_root() is deprecated and meson.project_source_root()/meson.project_global_root() should be used instead. But what @SOURCE_ROOT@ is set to is not clearly documented. I think it is set to meson.source_root(), i.e. meson.global_source_root(). So meson.project_source_root() should be used here too, like below.

Makefile Outdated
endef

builddir := $(shell $(PYTHON) -c '$(buildscript)')
BUILD_DIR = _build
Copy link
Member

Choose a reason for hiding this comment

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

Please keep build.

meson.build Outdated
# Common compile arguments matching setup.py
common_c_args = [
'-std=c99',
'-Werror=implicit-function-declaration',
Copy link
Member

Choose a reason for hiding this comment

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

gcc.1 says

This warning is enabled by default, as an error, in C99

So it can be dropped since we use c_std=c99.

@@ -0,0 +1,3 @@
[meson.build]
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer this file to be added first, so that the meson file is not reformatted in the middle of the series.

Comment on lines +66 to +78
source_files = [cast(Path, args.include_dir).joinpath('systemd/sd-messages.h'), cast(Path, args.source_dir).joinpath('id128-defines.h')]
print(f'To extract symbols from {source_files}...', file=sys.stderr)
source_lines = tuple(itertools.chain.from_iterable(extract_define_lines(file) for file in source_files))
print(f'Read {len(source_lines)} lines.', file=sys.stderr)
symbols = tuple(sorted(frozenset(s for line in source_lines if (s := extract_symbol(line)))))
print(f'Found {len(symbols)} unique symbols.', file=sys.stderr)
converted_lines = tuple(build_line_for_constant(s) for s in symbols)
print(f'Converted to {len(converted_lines)} lines.', file=sys.stderr)
dest_file = cast(Path, args.source_dir).joinpath('id128-constants.h')
dest_file.write_text('\n'.join(converted_lines) + '\n')
print(f'Wrote {len(converted_lines)} constants to {dest_file}.', file=sys.stderr)
update_docs(cast(Path, args.source_dir), symbols)
print('🎉 Done.', file=sys.stderr)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this was a nice simple script… I don't know exactly what is happening here, but in any case, please don't make changes like mixed with changes to the build system.

Copy link
Author

Choose a reason for hiding this comment

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

I have to rewrite this when droping Makefile and move the jobs to Meson. This script was a chain of Bash commands which doesn't fit well in Meson.

@hongquan
Copy link
Author

Thanks for commenting, @keszybz . I'm willing to redo this. Do we need to split to smaller PRs?
About etags. It is originally called etags in Makefile. But then I tried to make the GitHub install.yml workflow works, trying to install etags to the Linux distro and turn out that the package name in all distros are ctags, not etags. So I rename to ctags to avoid confusion.

@hongquan
Copy link
Author

Superseded by #148 .

@hongquan hongquan closed this Jun 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants