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

Rework of build system to use more modern standards #153

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

Conversation

SlouchyButton
Copy link
Collaborator

@SlouchyButton SlouchyButton commented Sep 12, 2024

Fixes: #152
Fixes: #149

This PR will probably not work on first try and there will need to be modifications here and there.

I tried to modify as little as possible while also cleaning parts of the app. Some things that are not needed anymore are removed. This also includes changes to COPR configuration which should already be applied and cleanup of GitHub webhooks.

Documentation wasn't built since 2023, so the webhook was refreshed and .readthedocs.yaml file added. I think this should fix the automatic documentation build, as the absence of the configuration file was why the last build failed a year ago. (https://readthedocs.org/projects/distgen/builds/21386423/)

Due to this modification, the spec file template for COPR had to be changed and spec file on https://src.fedoraproject.org/rpms/distgen will have to be changed as well.

Overview of changes:

  • Migrate to pyproject.toml from setup.py
    • This includes fully removing the setup.py file as it is not needed and relying on pyproject.toml for build only
    • Migrate manpage config to pyproject.toml from setup.cfg
    • Migration of flake8 not yet possible due to it being a bit flaky implementing support for pyproject.toml configuration (see pyproject.toml (PEP 518) support PyCQA/flake8#234)
  • Move parsing of command line arguments to different file from rest of the CLI interface for distgen (Thanks for help @praiskup)
    • __main__.py is the CLI interface now, cli_parser.py contains the ArgumentParser object only
    • This allows manpage generator to load the file without crashing on distgen module import
    • The way manpage generator work is that it loads the file with ArgumentParser object and gets all the information needed for manual page from that object
    • When this object shares the same file with the rest of the distgen CLI interface (which contains from distgen.X import X), the process of loading the file in manpage generator triggers the import, which is not possible due to distgen not being available during the build of distgen
    • Inspired by https://github.com/fedora-copr/resalloc-ibm-cloud
  • Move bin/dg to distgen/__main__.py (Thanks for help @duzda)
    • This allows use of python -m distgen to invoke the CLI interface and provides overall more standardized approach to modules
    • Also allows us to just use distgen.__main__:main as an entry point for script dg in pyproject.toml so preparing dg as an executable and placing it in /bin is done for us by the build system
  • Use version from pyproject.toml everywhere
    • After searching possible options for version management, combination of few options were used
    • We do not need (but possibly can use) automatic versioning (such as setuptools-scm), but the recommended approach to reusing version generated by setuptools-scm is used in a few places
    • cli_parser uses version() function from importlib.metadata, but also catches possible exception
      • This exception is thrown when building manpage from the CLI interface, due to distgen module not being present
      • When user invokes dg --version, the distgen module is (obviously) present, so we use the version directly from the module metadata
    • Similar approach is used in documentation, but without the exception catching as I think the module will be present when generating the docs in the Read the Docs
      • This is possibly one of the things that might still be broken
      • IDK if Read the Docs will install the distgen if not this will have to be resolved
    • Getting the version in the Makefile generating the spec file is a bit more hacky, but the current approach implies that distgen that is being built is also installed as a module, so the version is fetched by python3 -m distgen --version | grep -o '[0-9]*\.[0-9]*'
  • SPEC file template changed to reflect new build system
    • Since use of setup.py sdist can not be used anymore, and setup.py doesn't even exist now, the building process had to be reworked for fedora too
    • Modifications to the spec file are reflecting current state of building python packages for fedora
    • Inspiration came from multiple python packages in fedora currently as well as the Fedora packaging guidelines
    • Whole build is managed by macros used to build python packages instead of the former manual approach
    • This also means that the custom test suite was removed from being used in specfile, because the distgen package is not installed when building and due to the more streamlined approach to the CLI interface, it is not possible to call dg without the package being installed now.
    • This ideally is only temporary, and we can figure better solution here, I didn't find any, but would like to run the custom testsuite if possible
  • COPR automated build changed to allow grabbing version directly from the package
    • COPR won't depend on distgen now and will use the currently built version of distgen to prepare the specfile
    • This also have a benefit of having the ability to easily grab the version directly from the package metadata (pyproject.toml)
  • GitHub workflow for PyPi publish updated
  • Miscellaneous cleanups and refreshments
    • Badge for travis CI in readme removed
    • Badge for Fedora Repo added
    • Badge for COPR added
    • Authors updated

@SlouchyButton SlouchyButton self-assigned this Sep 12, 2024
distgen/__main__.py Fixed Show fixed Hide fixed
distgen/__main__.py Dismissed Show resolved Hide resolved
distgen/__main__.py Dismissed Show dismissed Hide dismissed
distgen/__main__.py Dismissed Show dismissed Hide dismissed
distgen/__main__.py Dismissed Show dismissed Hide dismissed
distgen/cli_parser.py Fixed Show fixed Hide fixed
distgen/cli_parser.py Fixed Show fixed Hide fixed
distgen/cli_parser.py Fixed Show fixed Hide fixed
distgen/cli_parser.py Fixed Show fixed Hide fixed
distgen/cli_parser.py Fixed Show resolved Hide resolved
@SlouchyButton
Copy link
Collaborator Author

Copr build succeeds on fedora now but fails for RHELs.

It seems that it fails to parse pyproject.toml file due to missing tomli module:
Import error: No module named 'tomli'

Although based on the log, the build dependency should be there correctly and it actually does install later:

+ '[' -f pyproject.toml ']'
+ echo '(python3dist(tomli) if python3-devel < 3.11)'
...
Installed:
  python3-pip-21.2.3-8.el9.noarch        python3-tomli-2.0.1-5.el9.noarch       
...
Package python3-tomli-2.0.1-5.el9.noarch is already installed.

But very obviously the build system fails to parse the pyproject.toml file, because later in the log the distgen is being refered to as UNKNOWN-0.0.0

Created wheel for UNKNOWN: filename=UNKNOWN-0.0.0-py3-none-any.whl size=8334 sha256=f90f61f871aee9ca9b2238ff73020d4b7944ad2d796bc3a59aff2d1320374dc0
Stored in directory: /builddir/.cache/pip/wheels/f6/c5/33/887f3d1123f0c71a734f5d796f6ee0b373bb0dc61964f85e26
Successfully built UNKNOWN

Eventually this is the reason why the build inevitably fails because:
ValueError: Globs did not match any module: distgen

Is this error on my side in configuring the spec file, or possibly an error somewhere else or a bug?

@praiskup
Copy link
Member

Wow, that's a huge pile of good work, thank you! 🎉 I barely can concentrate enough - some of those changes would be better in separate PRs, but it doesn't make sense to overcomplicate this contribution now. Just pardon my rather quick or maybe wrong review notes (the context is too big).

This is a low-prio note, as I do not maintain this anymore: it's a pity we are losing support for EPEL8+ with the new format of the upstream spec file. And if we drop the support here upstream, we drop CI for it, and it probably never gets updated in real EPEL in the future.

distgen/__main__.py Dismissed Show resolved Hide resolved
distgen/cli_parser.py Fixed Show resolved Hide resolved
# The full version, including alpha/beta/rc tags.
release = dg_version
release = get_version("distgen")
Copy link
Member

Choose a reason for hiding this comment

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

this will provide invalid versions from git-head

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fair, but so would the previous approach, right? When it was hardcoded to the file.

Using the https://pypi.org/project/setuptools-scm/ I mentioned above might help with this, right? Also, might make the custom version generation for push/PR builds in the copr script also redundant, as if the build is made from a commit that is not being tagged with version it will be based on commit hash.

Copy link
Member

Choose a reason for hiding this comment

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

From git-head it worked fine, if the cwd is on python's path.

pyproject.toml Outdated Show resolved Hide resolved
rpm/Makefile Show resolved Hide resolved
rpm/distgen.spec.dg Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
rpm/Makefile Outdated Show resolved Hide resolved
@SlouchyButton
Copy link
Collaborator Author

some of those changes would be better in separate PRs

I totally agree, but splitting it at this point would just be extra work, I feel like. Originally it wasn't meant to be such a huge PR, but since the change of build system meant, so many linked changes in spec file, copr etc. it kinda just slowly got huge.

it's a pity we are losing support for EPEL8+ with the new format of the upstream spec file

This wasn't my goal, tho. I actually didn't know it wouldn't work, and it seems like it kinda should? Since the spec file tries to build, see my comment above #153 (comment), but possibly fails due to missing tomli package, that is installed.

I just went based on current Fedora docs and how are other python packages packaged. Anyway, I would love to keep the support for EPEL, but I will very probably need help. Or we can rethink whether the support for EPEL is required and whether pypi install could be used in those cases. Since we are planning to onboard this on packit, I don't even know if packit supports EPEL anyway.

@praiskup
Copy link
Member

I just went based on current Fedora docs and how are other python packages packaged. Anyway, I would love to keep the support for EPEL but I will very probably need help.

The dynamic build dependencies is a relatively new thing in RPM ecosystem; you have to keep the good-old static buildrequires list.

Also inform about module API change - removed version file
Some of the new functionalities that are present in Fedora and are
needed for distgen to build using the refreshed build system are not
present in EPEL.

This means that we have to rollback some changes to allow build on
EPEL. This shouldn't impact functionality of the application, but
keeping it in the main repository would create mess in the codebase.
For that reason the rollbacks are handled by patches in spec file.
@praiskup
Copy link
Member

I'd be curious to see what the Python maintainers think about this approach :) have you tried to consult this?

It seems that Python on RHEL 8 is not prepared for projects that moved to pyproject.toml, and if the project builds/installs fine with the old scripting - I'd just stick with the old build system (because it "just works", even for RHEL 8). At least that's how I read this experiment... Isn't there some EL 8 cookbook for the pyproject.toml-enabled projects?

@SlouchyButton
Copy link
Collaborator Author

SlouchyButton commented Sep 17, 2024

After further discussion with @phracek and mhroncok we came to a conclusion that due to a very outdated nature of EPELs and missing dependencies in EPEL built for python3.12 it would be too difficult to set it up with no additional benefit for us.

We are not using distgen from EPEL, since most of our internal use comes from PyPI published version of distgen, it doesn't make sense requesting all the dependencies for EPEL.

Despite EPEL build being possible with the patches, they are not a clean solution and could create more complications down the road.

EPEL build would also be possible using up-to-date version of python in RHEL8 and RHEL9, but this comes with a problem of missing dependencies (distro, jinja2 for EPEL9 and argparse-manpage for both) compatible with python3.12.

Since there is no reason to support EPEL from DB team point of view, we will be dropping the support. distgen will stay on epel in the currently published version that should be working, and no more updates will be published there.

Last commit of this PR reverts the EPEL support changes, resulting in a clean spec file only working on Fedora for now and no patches.

@praiskup
Copy link
Member

We are not using distgen from EPEL, since most of our internal use comes from PyPI published version of distgen, it doesn't make sense requesting all the dependencies for EPEL.

And nobody else will be in the future 🤷 and that's what I think is a pity! Spoiler: /me is a distribution developer who always tries to integrate against the distributions we support (EPEL 7 is not in the equation, that's why I proposed #150). However many users care about EPEL 8+. And the support is cheaper than doing the move.

Note that this tool has "distribution" in its name, yet we are opting-in to a development model that disallows us to ship "natively" into the distributions we support. "Doing nothing" for now (because there's no solution) is just as easy as #154, and while seemingly lame (but if there's no better python solution?) that's what I think is a reasonable compromise.

But /me hides now.

@SlouchyButton
Copy link
Collaborator Author

And the support is cheaper than doing the move.

It is not, the move is already done via this PR, there is nothing to be done. On the other way, not doing the move will require us to revisit this again and again once more parts will be deprecated or removed. Using python setup.py sdist for build is already deprecated and could stop working anytime. Since setup.cfg is preferred way over setup.py config, it can also be messed up in different ways by updates. And both of them ale more of a legacy thing with pyproject.toml being standard now - even in the eyes of setuptools devs.

"Doing nothing" for now (because there's no solution) is just as easy as #154, and while seemingly lame

It will require us to do more maintenance on a project down the road, which we are not prioritizing.

but if there's no better python solution?

That's not entirely true. Only thing blocking distgen from being packaged to EPEL8 and 9 is those three missing dependencies afaik. It can be packaged, it just doesn't make sense to deal with pushing all those packages to EPEL when we don't even use it from it.

If someone needs the EPEL support, it is possible to include it, and we can revisit this when such time comes. It is as easy as opening bugzilla ticket on distgen package in fedora requesting new version and if some free times will allow us we can address it.

@phracek
Copy link
Collaborator

phracek commented Sep 18, 2024

My approach, or from my point of view, I would release version to PyPi and not to EPEL8,9 and let's use standard Python mechanism, in case customers would like to install distgen, then let's use directly pip3.12 install distgen. In our case, like containers, we use PyPi in all workflows. distgen EPEL8 installation is not used at all.

PyPi works properly in all hosts, like RHEL9, RHEL8, CentOS 10 where python3.12 is already present. Let's support only up2date python versions. Lower versions are a bit outdated.

@praiskup
Copy link
Member

While my vote is probably not important, I'm -1 for this PR as is, because it brings a distro drop for no real benefits, sorry. Yes, it is nice to use modern tooling, but it simply doesn't match the philosophy of this project (poor author's thought). This was a tool for distro developers, trying to make the distro differences smaller. Now we claim the distro differences are so big that it is impossible to package it, 🤷

I would release version to PyPi and not to EPEL8,9 and let's use standard Python mechanism ...

I'm not in the same camp here; I prefer dogfooding RPMs wherever possible.

@praiskup
Copy link
Member

praiskup commented Sep 18, 2024

It will require us to do more maintenance on a project down the road, which we are not prioritizing.

Can you elaborate? I can't offer much more than what I gave to this project in the last years (= hobby time).
But I don't see any challenges right now.

Only thing blocking distgen from being packaged to EPEL8 and 9

There's nothing blocking from it being packaged, it is already packaged :-)

those three missing dependencies

What you describe is moving EPEL 8+ against Python 3.12, which seems not worth it (negatives: it brings non-standard python stack with itself when installed).

@SlouchyButton
Copy link
Collaborator Author

But I don't see any challenges right now.

The way distgen is being built currently is deprected in newer standards of python. The new building methods caused the issue to happen. Since it is deprecated in python docs, there will be more breaking changes in the future. Each breaking change will force us to revisit this project, search for the issue and fix it. They will happen because use of setup.py sdist is deprecated and use of setup.py and setup.cfg are 'legacy' ways. Setuptools docs mention that they will continue to work for forceable future, but using pyproject.toml is the new standard way that will continue to be supported. (This does not include use of python setup.py sdist which is deprecated now and can stop working at any moment - this would in itself be a problem for RHEL 8 as that doesn't even have build module in default python)

There's nothing blocking from it being packaged

I'll paraphrase myself, those 3 things are blocking distgen in the state that it would be in after merging this PR.

It is packaged in EPEL and what I am proposing is that it will stay in the EPEL in the same version it is now. And if someone actually uses the EPEL version and will be in the need of a newer version, they can file bugzilla and request the update, and we can revisit this. This is standard process with EPEL and there are a lot of packages that aren't updated in EPEL automatically.

What you describe is moving EPEL 8+ against Python 3.12

I don't think It's unreasonable to want to use up-to-date python. Installing 'full new python stack' just doesn't seem as dramatic for me, it is 65 Mb installed size for python, pip and setuptools. And if I base this on our use case, they will have the newer python installed anyway for other packages, like we do for up to date PyTest. What I mean by this is that from my point of view, there is a high possibility that those users already have python3.12 installed if they already work with python or use other python libraries/tools.

And if you really needed up to date distgen you can just install it from PyPi anyway - I have tested this PR it will work correctly with default version of python in RHEL 9 and more up to date one in RHEL 8.

How I see this migration is that we do it once and correctly to prevent the need to revisit this once deprecated features get removed and/or legacy features get deprecated. What I feel from any other approach to this is that it is just bandaging the app instead of moving it forward.

None of those changes are bleeding edge either, the blog post informing about setup.py sdist being outdated and deprecated is from 2021 and that same blog post mentions that 'direct invocations of setup.py' were unmaintained for years already. And those 'new' PEP 517 and PEP 518 standards are from 2015 and 2016 respectively. So not new.

From how I see this, the way distgen is being built is deprecated, and even if we tried to fix it using a non pyproject.toml approach we would still end up with a legacy non-standard way of doing things. It will break, and there will be need to revisit this if we won't change it now. When the time inevitably comes, nothing will be different. We will come, after few days of searching for the problem, to the same conclusion and since there is no way that RHEL will get a new updated python stack, and there are no plans on backporting setuptools even for RHEL 9, the same 'but what about EPEL' question will rise. With no new solution.

@praiskup
Copy link
Member

This is so ineffective, and this is not a "battle" I want/need to win! So I
don't plan to react more to save everyone's efforts :) but I'm -1 because I
already know this is not the right move. I'd suggest you wait a
month or two and reconsider without emotions at least. Then come back and make
the best decision you can → hit the merge button or don't.

There's no need to hurry with this, there's no immediate problem (#154 is
ready for merge).

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

Successfully merging this pull request may close these issues.

Migrate to pyproject.toml installing distgen from PyPi failed with six traceback
3 participants