Bug 1648728 (python-progressbar2) - Review Request: python-progressbar2 - A Progressbar library to provide visual progress to long running operations
Summary: Review Request: python-progressbar2 - A Progressbar library to provide visual...
Keywords:
Status: CLOSED RAWHIDE
Alias: python-progressbar2
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1661676
Blocks: fedora-neuro python-xnat
TreeView+ depends on / blocked
 
Reported: 2018-11-11 22:20 UTC by Ankur Sinha (FranciscoD)
Modified: 2018-12-28 12:37 UTC (History)
8 users (show)

Fixed In Version: python-progressbar2-3.39.2-1.fc30
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-12-28 12:37:27 UTC
zbyszek: fedora-review+


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Bugzilla 1218533 None NEW use new forked progressbar2? 2019-01-11 15:39:57 UTC
Red Hat Bugzilla 1654745 None NEW Please consider using progressbar2 2019-01-11 15:39:57 UTC
Red Hat Bugzilla 1654746 None CLOSED Please consider using progressbar2 2019-01-11 15:39:57 UTC
Red Hat Bugzilla 1654747 None ASSIGNED Please consider using progressbar2 2019-01-11 15:39:57 UTC

Internal Links: 1218533 1654745 1654746 1654747

Description Ankur Sinha (FranciscoD) 2018-11-11 22:20:51 UTC
Spec URL: https://ankursinha.fedorapeople.org/python-progressbar2/python-progressbar2.spec
SRPM URL: https://ankursinha.fedorapeople.org/python-progressbar2/python-progressbar2-3.38.0-1.fc29.src.rpm

Description: 
A text progress bar is typically used to display the progress of a long running
operation, providing a visual cue that processing is underway.

The ProgressBar class manages the current progress, and the format of the line
is given by a number of widgets. A widget is an object that may display
differently depending on the state of the progress bar. There are many types of
widgets:

- AbsoluteETA
- AdaptiveETA
- AdaptiveTransferSpeed
- AnimatedMarker
- Bar
- BouncingBar
- Counter
- CurrentTime
- DataSize
- DynamicMessage
- ETA
- FileTransferSpeed
- FormatCustomText
- FormatLabel
- Percentage
- ReverseBar
- RotatingMarker
- SimpleProgress
- Timer

The progressbar module is very easy to use, yet very powerful. It will also
automatically enable features like auto-resizing when the system supports it.}


Fedora Account System Username: ankursinha


Rawhide scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=30812638
F29 scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=30812833

Comment 1 Zbigniew Jędrzejewski-Szmek 2018-11-12 20:08:56 UTC
> PR filed
An URL would be nice.

> find . -name "*.pyc" -exec rm -fv '{}' \;
find . -name '*.pyc' -print -delete

I think that the list of widgets in the description could be removed. It's not necessary to the users of the package.

+ package name is OK
+ latest version
+ builds and installs OK
+ license is acceptable for Fedora (BSD)
+ license is specified correctly
+ fedora-review is happy (there's a lot of spam from rpmlint, but I think it's all bogus).

Package is APPROVED.

Comment 2 Ankur Sinha (FranciscoD) 2018-11-12 20:24:20 UTC
Thanks for the review, Zbigniew. I've updated the spec accordingly.

I'm worried that this conflicts with the python-progressbar package:

sudo dnf whatprovides '/usr/lib/python3.7/site-packages/progressbar'
Last metadata expiration check: 2:00:11 ago on Mon 12 Nov 2018 18:21:35 GMT.
python3-progressbar-2.3-13.20170808git32422c1.fc29.noarch : Text progressbar library for Python 3
Repo        : fedora
Matched from:
Filename    : /usr/lib/python3.7/site-packages/progressbar

I reckon that means we can't have both packages without some hacking here?

Comment 3 Zbigniew Jędrzejewski-Szmek 2018-11-12 21:18:11 UTC
Yeah, I didn't check this. This needs to be figured out somehow.

Comment 4 Zbigniew Jędrzejewski-Szmek 2018-11-12 22:11:46 UTC
Is this progressbar package incompatible with the other one?
If yes, then I think the best option would be to rename the module in this one to progressbar2. The imports in users would have to be adjusted to match. But this would be a simple 'sed -i' in %prep, so not too cumbersome.

Comment 5 Zbigniew Jędrzejewski-Szmek 2018-11-14 08:47:21 UTC
It seems that both this progressbar (https://github.com/WoLpH/python-progressbar) and the other progressbar (https://github.com/niltonvolpato/python-progressbar) are maintained. The API look similar, so it is actually possible that dependent packages could be fine with either.

I think it'd be better to keep just one package, to avoid work, if they are compatible enough. Maintainers of python-progressbar, what do you think?

Comment 6 Petr Viktorin 2018-11-14 09:13:18 UTC
The choice of naming the library differently than the imported module is really unfortunate, especially if the author knew a "progressbar" library already exists. The two cannot be installed simultaneously.

Progressbar 2 looks more featureful and tested, but progressbar is used by other packages (and probably users) in Fedora, so we need to be careful.

Can you get upstreams of Fedora packages that currently need progressbar to switch to progressbar2? Those are euca2ools, libtaskotron and python-bitmath, AFAICS.

Comment 7 Ankur Sinha (FranciscoD) 2018-11-14 13:44:05 UTC
I've asked progressbar2 upstream here: https://github.com/WoLpH/python-progressbar/issues/177

Hopefully progressbar2 will be compatible with the original and we wont have to make any downstream changes (or request upstreams to move etc.)

Comment 8 Petr Viktorin 2018-11-14 14:41:48 UTC
I'm not worried about them being compatible *now*.

Consider this scenario:
- We replace python-progressbar by progressbar2 in Fedora
- The original progressbar adds a new feature, which is not in progressbar2
- python-bitmath upstream starts depending on the new feature

In this case, we couldn't update python-bitmath in Fedora.

Comment 9 Ankur Sinha (FranciscoD) 2018-11-14 14:56:09 UTC
Sure, but in either case once we decide what upstream to follow, we'll have to request upstreams of whatever packages to switch to it. 

Would it be possible to ask the python-progressbar upstream to retire it, and add a notice requesting people to move to the newer one, which appears to have more features and is better maintained?

If this cannot be sorted at an upstream level, the downstream resorts will be to make the two conflict, or we carry downstream patches for lots of software to make the two installable not conflict. (Neither of these are attractive to me).

Comment 10 Petr Viktorin 2018-11-14 15:29:37 UTC
(In reply to Ankur Sinha (FranciscoD) from comment #9)
> Sure, but in either case once we decide what upstream to follow, we'll have
> to request upstreams of whatever packages to switch to it. 

Yes.
If one is clearly better than the other, it shouldn't be an issue, right?


> Would it be possible to ask the python-progressbar upstream to retire it,
> and add a notice requesting people to move to the newer one, which appears
> to have more features and is better maintained?

IMO it would be better to merge with the fork, since "progressbar" is clearly a better name than "progressbar2".

> If this cannot be sorted at an upstream level, the downstream resorts will
> be to make the two conflict, or we carry downstream patches for lots of
> software to make the two installable not conflict. (Neither of these are
> attractive to me).

Exactly.

Comment 11 Ankur Sinha (FranciscoD) 2018-11-14 15:51:43 UTC
(In reply to Petr Viktorin from comment #10)
> (In reply to Ankur Sinha (FranciscoD) from comment #9)
> > Sure, but in either case once we decide what upstream to follow, we'll have
> > to request upstreams of whatever packages to switch to it. 
> 
> Yes.
> If one is clearly better than the other, it shouldn't be an issue, right?

I hope not, but one can't say with upstreams XD

Here's what progressbar2 upstream said:

https://github.com/WoLpH/python-progressbar/issues/177#issuecomment-438706012

"Hi Ankur,

It seems the original module has gotten at least a bit of a revival, but I still see several issues going all the way back to 2010 (such as this one: niltonvolpato/python-progressbar#8 (comment)) so I'm inclined to say it's still not receiving all that much love.

Throughout the development I've always taken care to make the interface backwards compatible so it won't cause any issues for people that switch between the libraries. Since the original project didn't have any tests it's hard to know for certain, but I don't see any issues when running the original project examples.py script."

So:

- do we agree progressbar2 is more active and a better candidate?

- if we do, 
* should we use the progressbar2 upstream in Fedora's python-progressbar package:

** should we test to see if the packages that depend on the older version function properly with the new one without requiring any changes (that'd be awesome)?
*** If they do, can we notify upstreams and request them to work against progressbar2 in the future?
*** if they do not, can we request their upstreams to migrate?
** does this require some sort of change notification (or simply an e-mail to -devel)

> 
> > Would it be possible to ask the python-progressbar upstream to retire it,
> > and add a notice requesting people to move to the newer one, which appears
> > to have more features and is better maintained?
> 
> IMO it would be better to merge with the fork, since "progressbar" is
> clearly a better name than "progressbar2".
> 

This would be brilliant too---we could work on this once the aforementioned points are taken care of?

Comment 12 Ankur Sinha (FranciscoD) 2018-11-19 13:33:27 UTC
Hello,

Any thoughts on this?

Cheers!
Ankur

Comment 13 Zbigniew Jędrzejewski-Szmek 2018-11-19 13:38:24 UTC
Somebody should test euca2ools, libtaskotron and python-bitmath with progressbar2 installed in place of progressbar. If that works, things should be quite easy.

Comment 14 Ankur Sinha (FranciscoD) 2018-11-19 22:12:00 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #13)
> Somebody should test euca2ools, libtaskotron and python-bitmath with
> progressbar2 installed in place of progressbar. If that works, things should
> be quite easy.

I ran some quick tests in a virtual environment.

--

I can't get libtaskotron's tests to run even with the original progressbar module. I don't know how these tests are meant to be run. I get this with both. Someone else will have to check that up:

running test
Traceback (most recent call last):
  File "/usr/lib/python3.7/site-packages/_pytest/config/__init__.py", line 372, in _getconftestmodules
    return self._path2confmods[path]
KeyError: local('/home/asinha/dump/progressbar-test/libtaskotron/testing')

During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/usr/lib/python3.7/site-packages/_pytest/config/__init__.py", line 403, in _importconftest
    return self._conftestpath2mod[conftestpath]
KeyError: local('/home/asinha/dump/progressbar-test/libtaskotron/testing/conftest.py')

During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/usr/lib/python3.7/site-packages/_pytest/config/__init__.py", line 409, in _importconftest
    mod = conftestpath.pyimport()
  File "/usr/lib/python3.7/site-packages/py/_path/local.py", line 668, in pyimport
    __import__(modname)
  File "/usr/lib/python3.7/site-packages/_pytest/assertion/rewrite.py", line 226, in load_module
    py.builtin.exec_(co, mod.__dict__)
  File "/home/asinha/dump/progressbar-test/libtaskotron/testing/conftest.py", line 13, in <module>
    import libtaskotron.config
  File "/home/asinha/dump/progressbar-test/libtaskotron/libtaskotron/config.py", line 25, in <module>
    from libtaskotron import file_utils
  File "/home/asinha/dump/progressbar-test/libtaskotron/libtaskotron/file_utils.py", line 12, in <module>
    import progressbar
ModuleNotFoundError: No module named 'progressbar'
ERROR: could not load /home/asinha/dump/progressbar-test/libtaskotron/testing/conftest.py


--

bitmath looks like it needs patching:

.. 
running build_ext
Traceback (most recent call last):
  File "setup.py", line 81, in <module>
    'bitmath = bitmath:cli_script',
  File "/home/asinha/dump/progressbar2-virt/lib64/python3.7/site-packages/setuptools/__init__.py", line 140, in setup
    return distutils.core.setup(**attrs)
  File "/usr/lib64/python3.7/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/usr/lib64/python3.7/distutils/dist.py", line 966, in run_commands
    self.run_command(cmd)
  File "/usr/lib64/python3.7/distutils/dist.py", line 985, in run_command
    cmd_obj.run()
  File "/home/asinha/dump/progressbar2-virt/lib64/python3.7/site-packages/setuptools/command/test.py", line 228, in run
    self.run_tests()
  File "/home/asinha/dump/progressbar2-virt/lib64/python3.7/site-packages/setuptools/command/test.py", line 250, in run_tests
    exit=False,
  File "/usr/lib64/python3.7/unittest/main.py", line 100, in __init__
    self.parseArgs(argv)
  File "/usr/lib64/python3.7/unittest/main.py", line 124, in parseArgs
    self._do_discovery(argv[2:])
  File "/usr/lib64/python3.7/unittest/main.py", line 244, in _do_discovery
    self.createTests(from_discovery=True, Loader=Loader)
  File "/usr/lib64/python3.7/unittest/main.py", line 154, in createTests
    self.test = loader.discover(self.start, self.pattern, self.top)
  File "/usr/lib64/python3.7/unittest/loader.py", line 347, in discover
    tests = list(self._find_tests(start_dir, pattern))
  File "/usr/lib64/python3.7/unittest/loader.py", line 404, in _find_tests
    full_path, pattern, namespace)
  File "/usr/lib64/python3.7/unittest/loader.py", line 481, in _find_test_path
    tests = self.loadTestsFromModule(package, pattern=pattern)
  File "/home/asinha/dump/progressbar2-virt/lib64/python3.7/site-packages/setuptools/command/test.py", line 54, in loadTestsFromModule
    tests.append(self.loadTestsFromName(submodule))
  File "/usr/lib64/python3.7/unittest/loader.py", line 154, in loadTestsFromName
    module = __import__(module_name)
  File "/home/asinha/dump/progressbar-test/bitmath/bitmath/integrations.py", line 84, in <module>
    class BitmathFileTransferSpeed(progressbar.widgets.Widget):
AttributeError: module 'progressbar.widgets' has no attribute 'Widget'

...

euca2ools isn't py3 compatible from the looks of it? Will it be dropped in rawhide then?

Comment 15 Ankur Sinha (FranciscoD) 2018-11-29 15:08:10 UTC
While all of this is sorted, can I package progressbar2 up as a separate pacakge and add conflicts: python-progressbar?

I'd like to have one implementation of progressbar, but I do not have the cycles to patch the 3 packages that use the older implementation. So, this is a shorter, temporary solution.

Comment 16 Ankur Sinha (FranciscoD) 2018-11-29 15:15:19 UTC
I've filed bugs against the three packages requesting the maintainers to consider using progressbar2 also.

Comment 17 Ankur Sinha (FranciscoD) 2018-11-29 15:17:47 UTC
(In reply to Ankur Sinha (FranciscoD) from comment #15)
> While all of this is sorted, can I package progressbar2 up as a separate
> pacakge and add conflicts: python-progressbar?
> 

I meant---since this package is good (comment 1, 2, 3), could the review be approved again and may I import it with the conflicts? (Sorry, not enough tea/coffee yet).

Comment 18 Ankur Sinha (FranciscoD) 2018-11-29 16:18:24 UTC
- Libtaskotron maintainer is happy to consider migration once progressbar2 is in Fedora (https://bugzilla.redhat.com/show_bug.cgi?id=1654746#c1)

- python-bitmath upstream is also happy to migrate (https://bugzilla.redhat.com/show_bug.cgi?id=1654747#c1)

Petr, is this enough to make progressbar2 the default implementation, or should I import it as a different package based on this review?

Comment 19 Ankur Sinha (FranciscoD) 2018-12-06 16:33:20 UTC
Ping progressbar maintainers: thoughts?

Zbigniew, could you please re-approve the package and let me import it? If the progressbar maintainers decide to use progressbar2 in the future, I can obsolete the package. This is holding up other packages that use progressbar2.

Cheers,
Ankur

Comment 20 Petr Viktorin 2018-12-06 17:47:21 UTC
Hello, and apologies for the delay.

If the progressbar and progressbar2 upstreams diverge, we'll be in quite a tough spot. But for now, it seems progressbar2 is better maintained, and wants to keep compatibility with the original.

So, let's replace python3-progressbar by python3-progressbar2.
The python2 package is going away soon though; let's keep that as it is.

Ankur, if it works for you, can you:
- post a heads-up to Fedora-devel, CCing maintainers of the dependent packages and the other progressbar admins (tuanta, cdamian), saying what the plan is

- remove the python2-progressbar2 subpackage entirely from the spec (this should simplify it quite a bit!)

- make python3-progressbar2 replace python3-progressbar (using versioned Obsoletes & Provides)

- get it approved

- please, add me as a co-maintainer

- let me know so I can remove the python3-progressbar package

Comment 21 Ankur Sinha (FranciscoD) 2018-12-07 08:45:34 UTC
Hi Petr,

That sounds good. I'll work on it when I get back from work this evening.

Thanks again,
Ankur

Comment 22 Ankur Sinha (FranciscoD) 2018-12-08 00:58:16 UTC
* Announced to devel list
* FpGM requested a change request so I've done this now: https://fedoraproject.org/wiki/Changes/Python-progressbar2_as_default

Since we're changing the default, I *think* it's a system-wide change. I don't expect it to be an issue, though.

* Removed py2
* Obsolete python3-progressbar (I don't see any python-progressbar builds for f30?)

* Updated spec/srpm:
https://ankursinha.fedorapeople.org/python-progressbar2/python-progressbar2.spec
https://ankursinha.fedorapeople.org/python-progressbar2/python-progressbar2-3.38.0-1.fc29.src.rpm

The tests won't yet run because freezegun needs an update. Noted in spec.

Cheers!
Ankur

Comment 23 Cătălin George Feștilă 2018-12-08 22:41:43 UTC
I think will need to go on to python 3.x version 
I agree with python3-progressbar features.

Comment 24 Luis Bazan 2018-12-10 14:39:09 UTC
(In reply to Ankur Sinha (FranciscoD) from comment #22)
> * Announced to devel list
> * FpGM requested a change request so I've done this now:
> https://fedoraproject.org/wiki/Changes/Python-progressbar2_as_default
> 
> Since we're changing the default, I *think* it's a system-wide change. I
> don't expect it to be an issue, though.
> 
> * Removed py2
> * Obsolete python3-progressbar (I don't see any python-progressbar builds
> for f30?)
> 
> * Updated spec/srpm:
> https://ankursinha.fedorapeople.org/python-progressbar2/python-progressbar2.
> spec
> https://ankursinha.fedorapeople.org/python-progressbar2/python-progressbar2-
> 3.38.0-1.fc29.src.rpm
> 
> The tests won't yet run because freezegun needs an update. Noted in spec.
> 
> Cheers!
> Ankur

+1 agree

Cheers,

Comment 25 Ankur Sinha (FranciscoD) 2018-12-13 12:39:38 UTC
Hello,

Sorry for another ping.

The change notification went out a few days ago. No one has come forward with any concerns, so this seems to be quite a low impact change that we can proceed with now.

Zbigniew, would you please (re)review the package and approve it if things are OK so that I can go ahead and build?


Spec URL: https://ankursinha.fedorapeople.org/python-progressbar2/python-progressbar2.spec
SRPM URL: https://ankursinha.fedorapeople.org/python-progressbar2/python-progressbar2-3.38.0-1.fc29.src.rpm


Cheers,
Ankur

Comment 26 Zbigniew Jędrzejewski-Szmek 2018-12-18 21:05:07 UTC
Hi, sorry for the delay. Package is re-APPROVED.

I think Petr's plan is sound.

Comment 27 Ankur Sinha (FranciscoD) 2018-12-18 22:22:25 UTC
Thanks! SCM requested: https://pagure.io/releng/fedora-scm-requests/issue/9318

Since the obsoletes is in place, can I request branches for F28/F29 also? That'll break things for users of the dependent packages, no? We have one or two packages that depend on progressbar2 which we'd like to provide in F29 at least if possible.

Cheers!
Ankur

Comment 28 Gwyn Ciesla 2018-12-18 22:37:26 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/python-progressbar2

Comment 29 Ankur Sinha (FranciscoD) 2018-12-19 12:51:35 UTC
Here's where we are now:

- post a heads-up to Fedora-devel, CCing maintainers of the dependent packages and the other progressbar admins (tuanta, cdamian), saying what the plan is

Done

- remove the python2-progressbar2 subpackage entirely from the spec (this should simplify it quite a bit!)

Done

- make python3-progressbar2 replace python3-progressbar (using versioned Obsoletes & Provides)

Done

- get it approved

Done

- please, add me as a co-maintainer

Done

- let me know so I can remove the python3-progressbar package

Doing now ;)

Here's the rawhide build: https://koji.fedoraproject.org/koji/taskinfo?taskID=31532077

Can I request branches for F28/F29, do you think?

Cheers!

Comment 30 Petr Viktorin 2018-12-19 15:23:53 UTC
Please also add a virtual Provides for python3-progressbar: https://src.fedoraproject.org/rpms/python-progressbar2/pull-request/1
 

Let's keep this in Rawhide for some time, to test it. Can we revisit f28/29 about mid-January?

Comment 31 Ankur Sinha (FranciscoD) 2018-12-19 19:01:54 UTC
(In reply to Petr Viktorin from comment #30)
> Please also add a virtual Provides for python3-progressbar:
> https://src.fedoraproject.org/rpms/python-progressbar2/pull-request/1

Merged, new build kicked off.

> 
> Let's keep this in Rawhide for some time, to test it. Can we revisit f28/29
> about mid-January?

Sure, sounds good. I've also dropped a comment on the 3 bugs filed for the dependent packages letting them know that progressbar2 is now available in rawhide.

Comment 32 Petr Viktorin 2018-12-20 15:08:59 UTC
I removed the old python-progressbar's Python 3 package on Rawhide.


Note: I will have limited access to e-mail until the new year. But if something breaks in Rawhide, hopefully it can wait until January 2.

Comment 33 Miro Hrončok 2018-12-22 00:11:50 UTC
Something broke in Rawhide, it can wait until January 2.

https://bugzilla.redhat.com/show_bug.cgi?id=1661676

Comment 34 Zbigniew Jędrzejewski-Szmek 2018-12-28 12:37:27 UTC
#1661676 is fixed now. Let's close this one, since the package is in rawhide and if there are bugs, they should be tracked separately.


Note You need to log in before you can comment on or make changes to this bug.