Closed #1546.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1546#event-4626456817___
Rpm-maint mailing list
I'm closing this PR as it has been reopened against the newly separate
python-rpm-packaging project:
https://github.com/rpm-software-management/python-rpm-packaging/pull/7
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
This will seriously complicate my life as a maintainer. There are many good use
cases for doing %check on the content of %install.
Your suggestion is to do a fake install in %build and check that, but that 1.
won't test the actually installed stuff, 2. make me copy a possibly lots of
code (not
> We have it ready in Fedora:
> https://fedoraproject.org/wiki/Changes/PythonExtras
>
> We want to backport it here but we have been swamped with many other things.
>
> cc @torsava
I've opened a PR to upstream our Fedora full-blown handling of Extras:
https://githu
Indeed both choices are not perfect. However, the Python parts are getting
rather complex. We even have a full blown test suite for them in Fedora that we
would be able to finally upstream if there's a separate repo. So overall, :+1:
from me.
--
You are receiving this because you are
In Fedora we have adapted the pythondistdeps.py script (that generates
python3.Xdist(foo) dependencies) to also generate requirements on Python extras
(e.g. python3.Xdist(foo[bar])) whenever upstream metadata indicate such
dependency.
The script was also adapted to be able to process a new
Closed #1541.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1541#event-4337554786___
Rpm-maint mailing list
Apologies, I've made a mistake. I wanted to open a PR against my own fork to
hammer out the kinks first. Closing for now.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
All 166 tests are passing. @hroncok Take a look please.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
@torsava pushed 2 commits.
84c0a7f0101125eed9c350c428626ceaf84b2e02 [DO NOT MERGE THIS] Our downstream
test suite
3190112709309e7b2dbd744e8d74404b0418052d [DO NOT MERGE THIS] New test for
extras
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https
First two commits are upstreaming of relevant Fedora changes:
-
https://src.fedoraproject.org/rpms/python-rpm-generators/c/d1a02fdda78743c796fe5c01cbb5287a8f3c873b?branch=rawhide
-
https://src.fedoraproject.org/rpms/python-rpm-generators/c/098c48d46d7f9eceb4b41eddd0b20b31611c8e41?branch=rawhide
So, who has the power to merge this? @ffesti ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1317#issuecomment-748928961___
> Oh, I've been alerted by @torsava that my suggestions have been addressed.
> Nice, thanks! The code looks good to me. If the external tests still pass,
> let's ship it?
>
> We can probably test this in Fedora before merging if we want more testing
> (but this is not nece
@torsava commented on this pull request.
> +try:
+from importlib.metadata import PathDistribution
+except ImportError:
+from importlib_metadata import PathDistribution
+
+try:
+from pathlib import Path
+except ImportError:
+from pathlib2 import Path
+
+
+class Req(Requirem
@torsava approved this pull request.
Looks good to me!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1317#pullrequestreview
@torsava commented on this pull request.
> @@ -142,9 +210,23 @@ def convert(name, operator, version_id):
def normalize_name(name):
@s-t-e-v-e-n-k Thinking more on this, since we have to keep this function, and
since the class/static methods you currently use are used inside 2 differ
@torsava commented on this pull request.
> @@ -142,9 +210,23 @@ def convert(name, operator, version_id):
def normalize_name(name):
Ah, right. In that case, yeah, a comment would be great.
--
You are receiving this because you are subscribed to this thread.
Reply to this email direc
@torsava commented on this pull request.
> +@classmethod
+def normalize_name(klass, name):
+"""https://www.python.org/dev/peps/pep-0503/#normalized-names"";
+return re.sub(r'[-_.]+', '-', name).lower()
+
+@classmethod
+def legac
@torsava commented on this pull request.
Besides the two mostly cosmetic comments, I think it looks good!
> @@ -142,9 +210,23 @@ def convert(name, operator, version_id):
def normalize_name(name):
This function is now completely unused and can be removed.
> +@classmethod
+
@torsava commented on this pull request.
> from warnings import warn
+try:
+from importlib.metadata import PathDistribution
+except ImportError:
+from importlib_metadata import PathDistribution
+
+try:
+from pathlib import Path
+except ImportError:
+from pathlib2 import P
@torsava commented on this pull request.
> from warnings import warn
+try:
+from importlib.metadata import PathDistribution
+except ImportError:
+from importlib_metadata import PathDistribution
+
+try:
+from pathlib import Path
+except ImportError:
+from pathlib2 import P
@torsava commented on this pull request.
> from warnings import warn
+try:
+from importlib.metadata import PathDistribution
+except ImportError:
+from importlib_metadata import PathDistribution
+
+try:
+from pathlib import Path
+except ImportError:
+from pathlib2 import P
@torsava commented on this pull request.
> from warnings import warn
+try:
+from importlib.metadata import PathDistribution
+except ImportError:
+from importlib_metadata import PathDistribution
+
+try:
+from pathlib import Path
+except ImportError:
+from pathlib2 import P
@torsava commented on this pull request.
> from warnings import warn
+try:
+from importlib.metadata import PathDistribution
+except ImportError:
+from importlib_metadata import PathDistribution
+
+try:
+from pathlib import Path
+except ImportError:
+from pathlib2 import P
@torsava commented on this pull request.
> from warnings import warn
+try:
+from importlib.metadata import PathDistribution
+except ImportError:
+from importlib_metadata import PathDistribution
+
+try:
+from pathlib import Path
+except ImportError:
+from pathlib2 import P
@torsava commented on this pull request.
> if normalized_names_require_pep503:
-dep_normalized_name = normalize_name(dep.project_name)
+dep_normalized_name = normalize_name(dep.name)
e
@torsava commented on this pull request.
> if normalized_names_require_pep503:
-dep_normalized_name = normalize_name(dep.project_name)
+dep_normalized_name = normalize_name(dep.name)
e
@torsava commented on this pull request.
> else:
-dep_normalized_name = dep.key
+dep_normalized_name = dep.key.replace('_', '-')
The newest version looks good to me, and tests are passing.
The [documentat
@torsava commented on this pull request.
> -print('Conflicts:\t{} {} {}'.format(dep.key,
> '==', spec[1]))
+for dep in dist.requirements_for_extra(extra):
+for spec in dep.spe
@torsava commented on this pull request.
> else:
-dep_normalized_name = dep.key
+dep_normalized_name = dep.key.replace('_', '-')
In my testing, the previous setuptools version already has underscores replaced
in the `
@torsava commented on this pull request.
> else:
-dep_normalized_name = dep.key
+dep_normalized_name = dep.key.replace('_', '-')
Why do you still do the `replace` here, but not in the other places where you
use `dep.
@torsava commented on this pull request.
> +if isdir(path):
+path = dirname(path)
+res = re.search(r"/python(?P\d+\.\d+)/", path)
This breaks detection of Python version from the name of the egg-info file. And
I think you misunderstood me, I wa
> Latest push now has every test successfully run the script, however that one
> test still fails because there's a difference in behaviour between the script
> in this repo and in python-rpm-generators.
>
> ```
> -if '>' == operator:
> -# distutils does not behave this way,
@torsava requested changes on this pull request.
Hi @s-t-e-v-e-n-k ,
first of all, I'm really sorry the review took so long :(
I've pointed out some issues in the comments, mostly in the less-or-not-used
parts of the code.
The PR as a whole looks fantastic, great piece of engineering. And all
> Okay, this new push solves all but one of the test failures, which has the
> script failing to parse the requires.txt for pyreq2rpm.tests since it
> contains comments.
Ah, I was glad that pkg_resources allowed those comments for easier
orientation, but we can remove them if we must.
--
You
> Any updates on the test suite investigation? Is there anything I can do to
> help?
@s-t-e-v-e-n-k Apologies, I got buried under different priorities. I'll try to
get this in a few days.
If you'd like to see on your own, the test suite for `pythondistdeps.py` is
currently only in our Fedora
There are test failures, but mainly do to different capitalization of names.
Plus I haven't been able to run the comparison test metadata for some reason.
I'll take a deeper look.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on
Looks good to me!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1308#issuecomment-656673366___
Rpm-maint mailing list
The change looks good to me. And we've been using this in RHEL for a while with
good results.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
> I think this should set _with_check unless _without_check is defined already.
> Basically to have `%bcond_without check` by default without having to put it
> in all spec files. But still need to make sure that somebody defines
> `%bcond_without check`, this code won't override it.
That
I would like to avoid `nocheck`, because the common way to conditinalize would
be `%if %{without nocheck}` and that's just confusing to read.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
+1 this would be a great way to standardize on disabling tests!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
> There are 3 things I'd like to see fixed:
>
> * the traceback should say: `Cannot process Python package version: 0+unknown`
> * the build should abort on errors
> * the version is [actually
> valid](https://www.python.org/dev/peps/pep-0440/#local-version-identifiers)
First and last issue
And are you planning to include the relevant scripts from these different repos
into the releases of rpm?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
Thank you too, @pmatilai!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1242#issuecomment-634522612___
Rpm-maint mailing list
> This pull request **introduces 1 alert** when merging
> [592a6d5](https://github.com/rpm-software-management/rpm/commit/592a6d5980010c63eb76c31ddd8954fba9cbaa92)
> into
> [8734c1b](https://github.com/rpm-software-management/rpm/commit/8734c1b97e39e3c7d3ac8396c4d6a2733852545c)
> - [view on
>
I have modified the commits to take out the test suite and opened it as a new
PR https://github.com/rpm-software-management/rpm/pull/1242.
Am therefore closing this PR in favour of the new one.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or
Closed #1195.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1195#event-3374229415___
Rpm-maint mailing list
This is a modification of PR
https://github.com/rpm-software-management/rpm/pull/1195, where Ive
deleted all mentions of the test suite.
CC @pmatilai, @Conan-Kudo, @ffesti, @hroncok
You can view, comment on, or merge this pull request online at:
Ok, sounds good. I'll separate out the test suite.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
I would like to work on some more changes, for example to handle "extras", but
I'm not sure it's a good idea before this is merged.
CC @Conan-Kudo, @pmatilai, @ffesti
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
These proposed changes have been merged to Fedora rawhide:
https://src.fedoraproject.org/rpms/python-rpm-generators/pull-request/15
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
> That's what I'm been mulling over here. Pulling stuff from multiple places
> does complicate release-cutting considerably though (which tends to be hard
> enough as it is), and will mean that bugs will get reported on rpm, but the
> code will be someplace else, which I suspect would be a
I've merged the first fixup. Leaving the second one pending depending on
whether we decide to merge pythondistdeps here or split off to a new package.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
While the [discussion about possibly moving pythondistdeps to a different
repo](https://github.com/rpm-software-management/rpm/issues/1199) goes on:
Any technical issues or concerns with the PR?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly
I can see some benefits of having some standalone domain-specific repos, like a
repo for Python scripts. However, I think Python is one of the special cases,
splitting out all the scripts would seem to me an overkill. And if we split
only some and leave the rest in rpm, it's more mess than we
@torsava commented on this pull request.
> @@ -11,6 +11,10 @@
# RPM python dependency generator, using .egg-info/.egg-link/.dist-info data
#
+# Please know:
+# - Notes from an attempted rewrite from pkg_resources to importlib.metadata in
+# 2020 can be found in the message of the com
@torsava commented on this pull request.
> @@ -11,6 +11,10 @@
# RPM python dependency generator, using .egg-info/.egg-link/.dist-info data
#
+# Please know:
+# - Notes from an attempted rewrite from pkg_resources to importlib.metadata in
+# 2020 can be found in the message of the com
> Any new sources need to be EXTRA_DIST in Makefile.am's.
I've added the checked-in files to the `EXTRA_DIST` in the fixup commit. I
didn't add the automatically-downloaded metadata, as the test script will
automatically fetch them if missing. If it's desirable to have them inside
EXTRA_DIST
@torsava pushed 1 commit.
e7cc87a083366439c0450c720ed36475b09d fixup! scripts/pythondistdeps: Add
the tests to the readme and makefile
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1195/files
@torsava commented on this pull request.
> @@ -11,6 +11,10 @@
# RPM python dependency generator, using .egg-info/.egg-link/.dist-info data
#
+# Please know:
+# - Notes from an attempted rewrite from pkg_resources to importlib.metadata in
+# 2020 can be found in the message of the com
@torsava pushed 1 commit.
96be70abdb8f5b7633fb41829a8b5d496a466e03 fixup! scripts/pythondistdeps: Add
option to generate major-version provides only for specified Python versions
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/rpm
@torsava commented on this pull request.
> +
+
+if __name__ == "__main__":
+"""To allow this script to be importable (and its classes/functions
+ reused), actions are performed only when run as a main script."""
+
+parser = argpa
> Any new sources need to be EXTRA_DIST in Makefile.am's. For a moment I was
> puzzling as to how come this is passing dist-check when the added sources are
> missing in the release tarball, but then realized the added tests are not
> getting run in _our_ test-suite. Which to me kinda steps
@torsava commented on this pull request.
> +
+
+if __name__ == "__main__":
+"""To allow this script to be importable (and its classes/functions
+ reused), actions are performed only when run as a main script."""
+
+parser = argpa
> **new alerts:**
>
> * 1 for Module is imported more than once
It's a lazy import so that if it's not needed, it doesn't slow down the
execution.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
Please do a review commit-by-commit. I changed indentation of a large part of
the code, so the overall diff is hard to read.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1195
-- Commit Summary --
*
> @torsava OK, let's wait for the next unexpected error. But wrapping
> everything in try-except and failing with useful error message is not a bad
> idea.
Ok, I'll add it to my PR.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directl
> the exception when the conversion fails is not telling anything useful
As this was an unexpected error, we don't know at what point in the code the
next unexpected error would strike. So the way I see it we would have to wrap
the whole machinery in `try..except` which isn't great, or do you
This very much breaks my pull request I'm preparing — it seems that once you
start working on pythondistdeps, everyone gets notification about it and starts
working on it too :) — but it's beautiful and I'm all for it!
--
You are receiving this because you are subscribed to this thread.
Reply
@torsava approved this pull request.
Code looks good, tests are passing.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1015#pullrequestreview
> I've pushed a change that should resolve the issues with trailing .0 versions.
And now all tests pass! +1
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
@gordonmessmer As for my tests, I'm preparing a few changes to
pythondistdeps.py, so I've added tests as well. The PR is still WIP.
https://github.com/torsava/rpm/pull/1
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
> This change incorporates code from
> https://github.com/gordonmessmer/pyreq2rpm/ which has a test suite of its own.
That's exactly where I took my test data. I have tested your PR and 84 out of
86 tests passed! These are the only 2 issues:
1. Input: foobar4~=2.0
Expected output:
> It would be nice to have the tests for these... But from the first glance
> this looks good to me.
I'm actually writing a test suite for `scripts/pythondistdeps`, I'll report
soon with the results.
--
You are receiving this because you are subscribed to this thread.
Reply to this email
Brainstorming:
* bcond_default_same + bcond_default_opposite
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
I would find it useful as well. However, we should really consider the naming,
as bconds are already confusing enough, this could become completely unreadable
to people not-in-the-know.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it
So seeing as no one has the resources to guide the larger patch, then unless
you find the simple shebang change acceptable, I suggest we close the issue.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
So, to conclude the discussion: I don't really have the know-how and time to
push through the more complicated change that @proyvind proposed. Would you be
interested in leading that, @proyvind ? I would help where I can.
--
You are receiving this because you are subscribed to this thread.
> As distros completes their migration to python 3 and no python 2 package
> installed on system, any py3k compatible script from packages in distro using
> /usr/bin/python as shebang will be left broken, for which there surely are
> quite a few.
That's exactly what you want and why Fedora,
> This makes no sense, /usr/bin/python is usually the linked to the binary of
> the default python version in use on distros, so hardcoding the version makes
> no sense.
That is a common misconception. While some distros went ahead and switched
`/usr/bin/python` to Python 3, most notably Arch
Thank *you!*
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/154#issuecomment-280033726___
Rpm-maint mailing list
torsava commented on this pull request.
> if not dist.py_version:
-warn("Version for {!r} has not been found".format(dist),
RuntimeWarning)
-continue
+# Try to parse the Python version from the path the metadata
+# resides
I've changed it to `\d+\.\d` so we don't get a Python Y2K equivalent. :)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
torsava commented on this pull request.
> if not dist.py_version:
-warn("Version for {!r} has not been found".format(dist),
RuntimeWarning)
-continue
+# Try to parse the Python version from the path the metadata
+# resides
85 matches
Mail list logo
