+1 from me

However, I would like to warn others, that this change is kinda theoretical because currently it does nothing :smile: . The _python_bytecompile_extra macro should work for Fedora >= 29, but in my experience, it does not have any effect on installed pyc/pyo files. It is defined there, though.

<mock-chroot> sh-4.4# rpm --eval %{_python_bytecompile_extra}
1
<mock-chroot> sh-4.4#

This PR sets it to 0, as suggested in the guidelines https://fedoraproject.org/wiki/Packaging:Python_Appendix#Manual_byte_compilation , so I think that we can merge it even without waiting for a RPM version where it works properly.

Yeah, the macro has been introduced in:

Name : rpm
Version : 4.14.2
Release : 0.rc1.1.fc29.2

as you confirmed in the rawhide mock chroot. So we could verify this change by actually building the rpms in the rawhide chroots and see if the build products contain pyc/pyo files or not.

That's what I tried many times before. But there was probably some user error from my side because I couldn't find any example where using the _python_bytecompile_extra makes any difference, because I found one now.

Let's see what happens when we modify the copr-frontend.spec according to this PR

[jkadlcik@chromie frontend]$ rpkg srpm
[jkadlcik@chromie frontend]$ mock -r fedora-29-x86_64 /tmp/rpkg/copr-frontend-11-w8cty2hm/copr-frontend-1.133.git.6.2cabceab.wtree.zmntll-1.fc26.s
[jkadlcik@chromie frontend]$ rpm2cpio /var/lib/mock/fedora-rawhide-x86_64/result/copr-frontend-1.133.git.6.2cabceab.wtree.zmntll-1.fc29.noarch.rpm
./usr/share/copr/coprs_frontend/alembic/fedora/env.pyc
./usr/share/copr/coprs_frontend/alembic/fedora/versions/14d5bf9ab362_enable_rawhide.pyc
./usr/share/copr/coprs_frontend/alembic/fedora/versions/3341bf554454_add_mageia_chroots.pyc
./usr/share/copr/coprs_frontend/alembic/fedora/versions/419a626c25e6_rename_rawhide_to_f26.pyc
...
./usr/share/copr/coprs_frontend/coprs/__pycache__/constants.cpython-36.pyc
./usr/share/copr/coprs_frontend/coprs/__pycache__/context_processors.cpython-36.pyc
./usr/share/copr/coprs_frontend/coprs/__pycache__/exceptions.cpython-36.pyc
./usr/share/copr/coprs_frontend/coprs/__pycache__/filters.cpython-36.pyc
...
./usr/share/copr/coprs_frontend/coprs/rest_api/resources/__pycache__
./usr/share/copr/coprs_frontend/coprs/rest_api/resources/__pycache__/__init__.cpython-36.pyc
./usr/share/copr/coprs_frontend/coprs/rest_api/resources/__pycache__/build.cpython-36.pyc
...

There are a lot of pyc files in the RPM file.

If we clean up the pyc files from the project directory and then build the package again

[jkadlcik@chromie frontend]$ sudo find ./ -name "*.py[co]" -delete
[jkadlcik@chromie frontend]$ rpm2cpio /var/lib/mock/fedora-rawhide-x86_64/result/copr-frontend-1.133.git.6.2cabceab.wtree.zmnt23-1.fc29.noarch.rpm
./usr/share/copr/coprs_frontend/alembic/fedora/env.pyc
./usr/share/copr/coprs_frontend/alembic/fedora/versions/14d5bf9ab362_enable_rawhide.pyc
./usr/share/copr/coprs_frontend/alembic/fedora/versions/3341bf554454_add_mageia_chroots.pyc
./usr/share/copr/coprs_frontend/alembic/fedora/versions/419a626c25e6_rename_rawhide_to_f26.pyc
./usr/share/copr/coprs_frontend/alembic/fedora/versions/8de41eec1d1_enabling_fedora_25_chroots.pyc

There are only pyc files in the alembic directory. (Which are also unwanted though)

And now the proof, that _python_bytecompile_extra did something ... Let's set it in the copr-frontend.spec to 1 and do the same thing again.

[jkadlcik@chromie frontend]$ find ./ -name "*.py[co]" -delete
[jkadlcik@chromie frontend]$ rpkg srpm
[jkadlcik@chromie frontend]$ mock -r fedora-29-x86_64 /tmp/rpkg/copr-frontend-15-kxfz6umg/copr-frontend-1.133.git.6.2cabceab.wtree.zmnucn-1.fc26.s
[jkadlcik@chromie frontend]$ rpm2cpio /var/lib/mock/fedora-rawhide-x86_64/result/copr-frontend- |cpio -t |grep pyc
... 
./usr/share/copr/coprs_frontend/coprs/views/__pycache__/misc.cpython-37.pyc
./usr/share/copr/coprs_frontend/coprs/views/admin_ns/__pycache__/__init__.cpython-37.opt-1.pyc
./usr/share/copr/coprs_frontend/coprs/views/admin_ns/__pycache__/__init__.cpython-37.pyc
./usr/share/copr/coprs_frontend/coprs/views/admin_ns/__pycache__/admin_general.cpython-37.opt-1.pyc
./usr/share/copr/coprs_frontend/coprs/views/admin_ns/__pycache__/admin_general.cpython-37.pyc
./usr/share/copr/coprs_frontend/coprs/views/api_ns/__pycache__/__init__.cpython-37.opt-1.pyc
./usr/share/copr/coprs_frontend/coprs/views/api_ns/__pycache__/__init__.cpython-37.pyc

There are a lot of pyc files again.

(Please note that there is no python3-modulemd for fedora 29, so to test this, I needed to temporairly delete it from BuildRequires and remove a %check section).

So, I guess we need to figure out how to get rid of the pyc files in the alembic directory.

Yes, thanks for testing it. We also need to switch off bytecompiling for doc generation, which generates the alembic pyc files.

Server components should be clean of any .pyc files now (except keygen which actually uses /usr/lib/python* dirs). I also verified, the pyc files are not required to be present for the servers to run successfully.

I don't think we want to drop byte compilation at all. Per the fedora proposal, we should use %py_byte_compile manually.

@churchyard, can you have a quick look? IMO copr is clear example of files where we should preserve byte compilation even though those are outside from $PYTHONPATH. Those files are basically executed by httpd, etc.

edit: The app, for frontend package, is executed by:
WSGIScriptAlias / /usr/share/copr/coprs_frontend/application

The question is, whether the files are imported or not. If they are, you need to bytecompile manually. If not, you don't want to bytecompile at all. I can check the source code to find the answer, however I suspect you might already know this.

OK, so the files in /usr/share/copr/coprs_frontend/ that are imported (such as the coprs directory) need to be byte compiled. yet files like manage.py don't. Compiling the entire folder however does no harm I guess.

OK, so the files in /usr/share/copr/coprs_frontend/ that are imported (sucha s the coprs directory) need to be byte compiled. yet files like manage.py don't. Compiling the entire folder however does no harm I guess.

Why do they need to be byte-compiled? I checked that the frontend and backend server works without any previous .pyc or .pyo files present. Is performance the reason why they need to be byte-compiled?

IMO yes, performance, since python will at least attempt to create the files all the time.

.. which in turn means that the files would be created if the scripts are executed (even accidentally) by UID=0. You easily get unpackaged files under /usr....

Exactly that second part.

I've added the manual byte-compilation for backend, frontend, and copr-dist-git. I've also cleaned up PYTHONDONTWRITEBYTECODE where it was not needed e.g. because -B python3 flag is used later. I've also removed %global _python_bytecompile_extra 0 from copr-keygen.spec. I've added BR: python3-devel into copr-dist-git so that the %py_byte_compile macro is present during build. Anything else should be done here?

What was the motivation for the removal of %global _python_bytecompile_extra 0?

What was the motivation for the removal of %global _python_bytecompile_extra 0?

copr-keygen actually uses /usr/lib/pythonX.Y/site_packages path to store its files.

%global _python_bytecompile_extra 0 stops automagic pybytecompilatyion outside of /usr/lib/pythonX.Y/site-packages not in them.

%global _python_bytecompile_extra 0 stops automagic pybytecompilatyion outside of /usr/lib/pythonX.Y/site-packages not in them.

Ok, so if a package does not store anything outside of /usr/lib/pythonX.Y/site-packages, then %global _python_bytecompile_extra 0 should not be needed and therefore can be removed.

There is only %{buildroot}%{_datadir}/copr-keygen/application.py, which is then used inside httpd config like this:

WSGIScriptAlias / /usr/share/copr-keygen/application.py

Is %global _python_bytecompile_extra 0 needed because of that?

Theoretically yes. In practice, if %{__python} (/usr/bin/python unless redefined in spec) is not in the buildroot, it is not needed. If you can guarantee that, you don't need %global _python_bytecompile_extra 0.

Theoretically yes. In practice, if %{__python} (/usr/bin/python unless redefined in spec) is not in the buildroot, it is not needed. If you can guarantee that, you don't need %global _python_bytecompile_extra 0.

Hm ok, I will add it back.

Added back. Also used python3 -B in the spec instead of the env var.

Why PYTHONDONTWRITEBYTECODE is needed here and above?

I see now it's equivalent to python3 -Bbelow, but why this is needed? I've never seen this before in spec file.

It is not needed I believe. I've just put it there to get the pyc files generated solely by %py_byte_compile macro so that it's clear where they are coming from. But given that the test suite is run after the pyc generation (which it is), then the pyc files are already generated at this point so PYTHONDONTWRITEBYTECODE should do just nothing.

I'd rather avoid that, there can not be mismatch because %py_byte_compile should only ever be called on %buildroot dir, OTOH all the test-related and build-related things should happen under %_builddir.

I don't mind keeping it there and I also don't mind removing it.

If you want me to remove it, then tell me exactly the places where I should remove it from because now it's made so that no tests run from spec file generate the pycs and also no doc generation so at least we know what's going on.

Since it's pretty nonstandard, and the generated pyc files really don't hurt us -- I'd prefer to drop all occurrences; the only reason is that it raises the question "why".

It's only preferrence, I don't mind if you prefer something else... just that it confuses me, so I guess it could confuse others.

I have additionally removed unneeded PYTHONDONTWRITEBYTECODE=1 in %check from copr-frontend.spec per comments. I prefer to keep it everywhere else. For doc generation it is needed, otherwise we get the .pyc files generated from it as a side effect, which might not be what we want.

I'm convinced that PYTHONDONWRITEBYTECODE (or python -B) is not needed at all, but I won't complicate it here now. So OK.

I'm a bit disapointed with the python change; I don't really understand why the use of _python_bytecompile_extra is needed, but I'll try to discuss on fedora devel.

I'd prefer having this as one commit, to not complicate git log detective work in future. I'd appreciate a note in commit why we use PYTHONDONTWRITEBYTECODE... because it is
weird (I checked all rawhide packages, and only those do it right now:

APLpy.spec
python-geoplot.spec
python-matplotlib.spec
python-mplcairo.spec
python-photutils.spec
python-pytest-mpl.spec
python-pytest-xdist.spec
python-setuptools.spec
waiverdb.spec

Overall, +1 if we document the "why" and make it one commit.

Huh, Pagure? Why I missed this...
@clime wrote:

I prefer to keep it everywhere else. For doc generation it is needed, otherwise we get the .pyc files generated from it as a side effect, which might not be what we want.

I've checked copr-frontend-doc, and even now I don't see neither py nor pyc files included.

Huh, Pagure? Why I missed this...
@clime wrote:

I prefer to keep it everywhere else. For doc generation it is needed, otherwise we get the .pyc files generated from it as a side effect, which might not be what we want.

I've checked copr-frontend-doc, and even now I don't see neither py nor pyc files included.

They won't be in copr-frontend-doc but in copr-frontend. I however tested this on copr-backend. Remove (not just comment, but remove) the line for manual pyc generation and remove PYTHONDONWRITEBYTECODE=1 in %build section for doc package. You will get pyc files in the resulting copr-backend rpm. If you return PYTHONDONWRITEBYTECODE=1, you won't get the pyc files there. Meaning that the pyc files are generated as a side product of doc generation, which is not what I want here because I want them to be generated by the manual macro only so that we know where the pyc files are coming from exactly.

Forgot to mention that you should first do rpkg clean -x to get rid of existing files in working tree, then build the srpm and build that srpm in rawhide so that _python_bytecompile_extra has the effect.

Of course, we could also just remove any existing pyc files before installing anything and then use the manual generation macro at the end but I am not exactly willing to that now because we should have an equivalent solution already.

I don't think it is a problem if the make creates *.pyc in %_builddir (!= %buildroot). There's no risk that it get's installed? But np.

Don't you want to put the PYTHONDONWRITEBYTECODE into Makefile instead of spec file? Someone might follow the docs and generate the docs manually..

I don't think it is a problem if the make creates *.pyc in %_builddir (!= %buildroot). There's no risk that it get's installed? But np.

The order of operation is important too, if you get the pyc files into %_builddir before the %install phase is run, then you get them installed. %check phase is run after install phase so, yes, there it matters that the tests are in builddir.

Don't you want to put the PYTHONDONWRITEBYTECODE into Makefile instead of spec file? Someone might follow the docs and generate the docs manually..

No, I think it's ok like this.

The order of operation is important too, if you get the pyc files into %_builddir before the %install phase is run, then you get them installed. %check phase is run after install phase so, yes, there it matters that the tests are in builddir.

Can you elaborate what files will be installed?

The order of operation is important too, if you get the pyc files into %_builddir before the %install phase is run, then you get them installed. %check phase is run after install phase so, yes, there it matters that the tests are in builddir.

Can you elaborate what files will be installed?

The .pyc files that got generated during the doc generation (build phase).

No, I think it's ok like this.

Would you mind explaining this?

The .pyc files that got generated during the doc generation (build phase).

I mean, what files concretely? The files which are compiled that way would be compiled and installed anyways, or?

FWIW, based on this trhead [1], I don't think we actually necessarily have to use %_python_bytecompile_extra at all. As said here, the only issue could be with /usr/bin/python installed in buildroot, and that would be (if it ever happened) low prio and temporary issue.

For the sake of correctness, I'd say the only thing we have to do with respect to the fedora change is to explicitly use %py_byte_compile with %__python3

[1] https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/IDDR65FMKZYZYLL6DGFEKFACC55PELW3/

You could also design the %files glob in a away that could render the package FTBFS if it ever happens.

@praiskup can we merge this or do you want to open a new minimal PR where it's done according to your findings?

@clime I'm fine to submit new PR, if you are OK with that.

@clime, but +1 if you want to merge this as is. I can cleanup later (there are other python 2/3 issues worth fixing..).

ok +1 for merging now and cleaning up later. Thanks!