devexp-db / distgen

Distribution oriented templating system
http://distgen.readthedocs.io/
GNU General Public License v2.0
18 stars 13 forks source link

Rework of build system to use more modern standards #153

Closed SlouchyButton closed 2 months ago

SlouchyButton commented 2 months ago

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:

SlouchyButton commented 2 months ago

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 commented 2 months ago

Wow, that's a huge pile of good work, thank you! :tada: 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.

SlouchyButton commented 2 months ago

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 https://github.com/devexp-db/distgen/pull/153#issuecomment-2346937605, 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 commented 2 months ago

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.

praiskup commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 :shrug: 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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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, :shrug:

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

SlouchyButton commented 2 months ago

After further discussion within the team, a new legacy branch will be created which will continue to support older python and setuptools. This branch will get PR #154 merged so it will be closer to this PR codebase.

This will allow EPEL to build using the older practices while keeping the main branch and 'default' up to date. Since the code of distgen haven't been changed by this PR (at least not significantly) potential merges from main to legacy should be straightforward.

By doing this we can still push updates for EPEL, while not having to deal with potentially more breaking changes in main branch.

Legacy will continue to be versioned '1.x', main will follow new major version '2.x'. This will allow us to also still do releases for both branches as PyPi should be able to pick the highest compatible version - so in case user installs distgen on RHEL8 default python - pip should even automatically resolve this and download newest '1.x' version from PyPi as version '2.x' needs newer python than is provided by default.

praiskup commented 2 months ago

After further discussion within the team, a new legacy branch

I don't think this is needed. Nobody will ever update it, because it will be legacy. I think there's no need to waste the effort because of me, at least.

This branch will get PR https://github.com/devexp-db/distgen/pull/154 merged

Ok, merged then.

praiskup commented 2 months ago

Then come back and make the best decision you can → hit the merge button or don't.

As I promised, /me disappears now. So taking back this "wait" suggestion. Feel free to go.