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

Reimplement pythondeps.sh as parametric macro generators #1153

Merged
merged 2 commits into from
Apr 14, 2020

Conversation

hroncok
Copy link
Contributor

@hroncok hroncok commented Mar 31, 2020

pythondeps.sh was written in shell and unlike the Python dist generators,
it uses no Python, it plainly determines the provide / requires from the path.
As the script was run for every Python file, we were potentially doing hundreds
of shelling outs to execute a script that calls grep and sed.

In Lua, this should be more efficient.

Fixes #1152

@hroncok
Copy link
Contributor Author

hroncok commented Mar 31, 2020

I have not yet tested this at all.

@pmatilai
Copy link
Member

pmatilai commented Apr 1, 2020

Cool!

You'll need to remove pythondeps.sh from scripts/Makefile.am though, that's what the CI is failing on. Also please avoid mixing whitespace changes with actual code changes, especially with %__python_path it's impossible to see offhand whether the actual line changes here or not.

pythondeps.sh was written in shell and unlike the Python dist generators,
it uses no Python, it plainly determines the provide / requires from the path.
As the script was run for every Python file, we were potentially doing hundreds
of shelling outs to execute a script that calls grep and sed.

In Lua, this should be more efficient.

Fixes rpm-software-management#1152
@hroncok
Copy link
Contributor Author

hroncok commented Apr 1, 2020

Done. Should I convert the indentation to tabs as well, commit another whitespace only tab-removal, or leave as is?

@hroncok
Copy link
Contributor Author

hroncok commented Apr 1, 2020

Some testing:

$ mock -r fedora-rawhide-x86_64 --enablerepo=local --init
$ mock -r fedora-rawhide-x86_64 --enablerepo=local --update
$ rpm -q --root ~/rpmbuild/mock/fedora-rawhide-x86_64/root/ rpm
rpm-4.15.90-0.git14971.1.fc33.x86_64

$ mock -r fedora-rawhide-x86_64 --copyin python.attr $(rpm --eval '%{_fileattrsdir}')/python.attr
INFO: copying python.attr to /var/lib/mock/fedora-rawhide-x86_64/root/usr/lib/rpm/fileattrs/python.attr


# the file is edited to use "xython"
$ egrep '=.+abi' /var/lib/mock/fedora-rawhide-x86_64/root/usr/lib/rpm/fileattrs/python.attr
        provides = path:gsub('.*/usr/bin/python(%d+%.%d+)', 'xython(abi) = %1')
        requires = path:gsub('.*/usr/lib%d*/python(%d+%.%d+)/.*', 'xython(abi) = %1')



[python-six (master)]$ fedpkg mockbuild -N

$ rpm -qp --requires results_python-six/1.14.0/1.fc33/python3-six-1.14.0-1.fc33.noarch.rpm  | grep abi
python(abi) = 3.8
xython(abi) = 3.8


$ sudo rm -f /var/lib/mock/fedora-rawhide-x86_64/root/usr/lib/rpm/fileattrs/pythondist.attr 

[python-six (master)]$ fedpkg mockbuild -N

$ rpm -qp --requires results_python-six/1.14.0/1.fc33/python3-six-1.14.0-1.fc33.noarch.rpm  | grep abi
xython(abi) = 3.8



[python3 (master)]$ fedpkg mockbuild -N --without optimizations --without tests --without computed_gotos --without debug_build

$ rpm -qp --provides results_python3/3.8.2/2.fc33/python3-3.8.2-2.fc33.x86_64.rpm | grep abi
python(abi) = 3.8
xython(abi) = 3.8

$ egrep Provides.+abi python3.spec
Provides: python(abi) = %{pybasever}

# removed it

[python3 (master *)]$ fedpkg mockbuild -N --without optimizations --without tests --without computed_gotos --without debug_build

$ rpm -qp --provides results_python3/3.8.2/2.fc33/python3-3.8.2-2.fc33.x86_64.rpm | grep abi
xython(abi) = 3.8

@hroncok
Copy link
Contributor Author

hroncok commented Apr 1, 2020

Testing has shown this seems to work for the cases where it is supposed to work.
I haven't really done any "this package looks like it is almost supposed to provide python(abi), but not really" testing, because I don't know such cases.

@pmatilai
Copy link
Member

pmatilai commented Apr 1, 2020

Should I convert the indentation to tabs as well, commit another whitespace only tab-removal, or leave as is?

Up to you.

In an attempt to get some timings for this thing:

  • moved pythondist.attr out of the way (that thing is expensive)
  • ran for p in $(rpm -qa 'name=python*'); do rpm -ql ${p}|./rpmdeps --requires |grep abi; done on my laptop with 181 python* packages with the old shell-version and this lua-macro version, both gave identical results

The shell version ran for 2m 13s, this version in 0m 18s. Of that 18s, the part of actual dependency generation is half at most. Nice job! 👍

@hroncok
Copy link
Contributor Author

hroncok commented Apr 1, 2020

The shell version ran for 2m 13s, this version in 0m 18s. Of that 18s, the part of actual dependency generation is half at most. Nice job!

Thanks. Do you want to test this in rawhide before we merge this, or merge and fix later if it appears broken somehow?

@pmatilai
Copy link
Member

pmatilai commented Apr 1, 2020

I think we can just merge and deal with possible fallout when it hits rawhide, that shouldn't be too far from now. Just letting this sit here for a day or two to give other Python folks a chance to comment.

@hroncok
Copy link
Contributor Author

hroncok commented Apr 1, 2020

In Fedora, we ship this from a different package. So it is not tight on when rpm will be updated but on when python-rpm-generators will.

@pmatilai
Copy link
Member

pmatilai commented Apr 1, 2020

Oh, right. Totally forgot about that. I certainly wont object if you want to give it a spin in rawhide before we hit the merge button here, but certainly not required either.

@hroncok
Copy link
Contributor Author

hroncok commented Apr 1, 2020

Is there a way to do something like:

Requires:       rpmlib(ParametricMacroGenerators)

Or do I have to do:

Requires:       rpm > 4.15.90-0

@pmatilai
Copy link
Member

pmatilai commented Apr 1, 2020

Unfortunately there's no meaningful way to track build-side rpm capabilities (just an RFE ticket at #717), so you'll just need to use a versioned dependency on rpm.

@hroncok
Copy link
Contributor Author

hroncok commented Apr 1, 2020

Copy link
Member

@Conan-Kudo Conan-Kudo left a comment

Choose a reason for hiding this comment

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

I acked this on the Fedora side, so here's the same ack on the upstream side. 😄

@pmatilai
Copy link
Member

Okay, no news is good news and nothing heard from Fedora in the 10 days this has been enabled there, lets just merge.

@pmatilai pmatilai merged commit 813858c into rpm-software-management:master Apr 14, 2020
@hroncok hroncok deleted the luapython branch April 14, 2020 09:07
@pmatilai
Copy link
Member

Oh and while I did compliment on the job, I didn't thank for it. So here goes: thanks! 😁

@hroncok
Copy link
Contributor Author

hroncok commented Apr 14, 2020

Thanks for being helpful.

hroncok added a commit to hroncok/rpm that referenced this pull request Apr 14, 2020
 - sync with Fedora
 - only match direct Python sitedir, no /opt/.../lib/python3.7 stuff
 - prep for multiple digits Python major releases (e.g. 12.1)
 - use the value of %_prefix

See also:

https://src.fedoraproject.org/rpms/python-rpm-generators/c/8eef42cbaa6ff0e5c006959fc06ec115ed5ca37b
https://lists.fedoraproject.org/archives/list/packaging@lists.fedoraproject.org/thread/UFKUM5UKCTNGIT3KJVYEI5VXPI23QMBN/
rpm-software-management#1153 (comment)
@hroncok hroncok mentioned this pull request Apr 14, 2020
jollaitbot pushed a commit to sailfishos-mirror/python-rpm-generators that referenced this pull request Apr 14, 2020
pythondeps.sh was written in shell and unlike the Python dist generators,
it uses no Python, it plainly determines the provide / requires from the path.
As the script was run for every Python file, we were potentially doing hundreds
of shelling outs to execute a script that calls grep and sed.

In Lua, this is much more efficient.

Some timings:
    rpm-software-management/rpm#1153 (comment)

Parametric macro generators require RPM 4.16+:
    https://fedoraproject.org/wiki/Changes/RPM-4.16

Fixes rpm-software-management/rpm#1152
Upstream PR: rpm-software-management/rpm#1153

Since this is intended for Fedora 33+ only, clean some old cruft.
pmatilai pushed a commit that referenced this pull request Apr 17, 2020
 - sync with Fedora
 - only match direct Python sitedir, no /opt/.../lib/python3.7 stuff
 - prep for multiple digits Python major releases (e.g. 12.1)
 - use the value of %_prefix

See also:

https://src.fedoraproject.org/rpms/python-rpm-generators/c/8eef42cbaa6ff0e5c006959fc06ec115ed5ca37b
https://lists.fedoraproject.org/archives/list/packaging@lists.fedoraproject.org/thread/UFKUM5UKCTNGIT3KJVYEI5VXPI23QMBN/
#1153 (comment)
hroncok added a commit to rpm-software-management/python-rpm-packaging that referenced this pull request Apr 7, 2021
 - sync with Fedora
 - only match direct Python sitedir, no /opt/.../lib/python3.7 stuff
 - prep for multiple digits Python major releases (e.g. 12.1)
 - use the value of %_prefix

See also:

https://src.fedoraproject.org/rpms/python-rpm-generators/c/8eef42cbaa6ff0e5c006959fc06ec115ed5ca37b
https://lists.fedoraproject.org/archives/list/packaging@lists.fedoraproject.org/thread/UFKUM5UKCTNGIT3KJVYEI5VXPI23QMBN/
rpm-software-management/rpm#1153 (comment)
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.

RFE: Reimplement pythondeps.sh as parametric macro generators
3 participants