Opened 6 years ago

Closed 3 years ago

#5001 enhancement closed fixed (fixed)

replace call to functions from the string module in twisted.scripts.tap2deb

Reported by: Juan A. Diaz Owned by: Thijs Triemstra
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Thijs Triemstra Branch: branches/str-methods-scripts-5001-3
branch-diff, diff-cov, branch-cov, buildbot
Author: thijs

Description

To make the port to python 3k more easy, I'm replacing all the calls to functions from the string module, for their equivalent from the str type object or calling directly to the built-in method of the string variable.

Attachments (2)

scripts.patch (698 bytes) - added by Juan A. Diaz 6 years ago.
patch
tap2deb-5001.patch (6.4 KB) - added by Thijs Triemstra 5 years ago.

Download all attachments as: .zip

Change History (35)

Changed 6 years ago by Juan A. Diaz

Attachment: scripts.patch added

patch

comment:1 Changed 6 years ago by Juan A. Diaz

Keywords: py3k, reviewpy3k review

comment:2 Changed 6 years ago by lvh

Owner: set to lvh
Status: newassigned

comment:3 Changed 6 years ago by lvh

Author: lvh
Branch: branches/str-methods-scripts-5001

(In [31460]) Branching to 'str-methods-scripts-5001'

comment:4 Changed 6 years ago by lvh

Keywords: review removed

This is so trivial and the buildbots are busy checking non-trivial things, I'm not going to wait for a build to complete here.

Added to my merge queue. Thanks as always for your patch :)

comment:5 Changed 6 years ago by Jean-Paul Calderone

This patch modifies untested code.

comment:6 Changed 6 years ago by lvh

Author: lvh
Owner: changed from lvh to Juan A. Diaz
Status: assignednew

What JP said, we should probably get a test here even though it's pretty obviously human-verifiable.

comment:7 Changed 5 years ago by Thijs Triemstra

Milestone: Python-3.x

Changed 5 years ago by Thijs Triemstra

Attachment: tap2deb-5001.patch added

comment:8 Changed 5 years ago by Thijs Triemstra

Cc: review added
Owner: Juan A. Diaz deleted

Attached patch adds tests for tap2deb and replaces the call to string.

comment:9 Changed 5 years ago by Thijs Triemstra

Author: thijs
Branch: branches/str-methods-scripts-5001branches/str-methods-scripts-5001-2

(In [35045]) Branching to 'str-methods-scripts-5001-2'

comment:10 Changed 5 years ago by Thijs Triemstra

(In [35047]) apply tap2deb-5001.patch and add news file, refs #5001

comment:11 Changed 5 years ago by Thijs Triemstra

(In [35053]) replace file with FilePath, fix tests. refs #5001

comment:12 Changed 5 years ago by Thijs Triemstra

Turns out the test was incorrect and not properly skipped which is fixed now, and file was replaced with FilePath as well. [buildbot.twistedmatrix.com/boxes-supported?branch=/branches/str-methods-scripts-5001-2 Build results]

comment:13 Changed 5 years ago by Thijs Triemstra

comment:14 Changed 4 years ago by Thijs Triemstra

Cc: review removed
Owner: set to Thijs Triemstra
Status: newassigned

comment:15 Changed 4 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: review added; py3k removed
Owner: Thijs Triemstra deleted
Status: assignednew
Summary: replace call to functions from the string module in scriptsreplace call to functions from the string module in twisted.scripts.tap2deb

9 months ago forgot to correctly put this branch up for review, this is take 2. Build results.

Also renaming the ticket since string's only used in tap2deb.

comment:16 Changed 4 years ago by therve

Keywords: review removed
Owner: set to Thijs Triemstra
  1. There are several twistedchecker errors:
************* Module twisted.scripts.tap2deb
C0103: 66,4:run: Invalid name "build_dir" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103: 71,4:run: Invalid name "debian_dir" (should match ([a-z_][a-zA-Z0-9]*)$)
W9402:292,0: The first letter of comment should be capitalized
W9501:199,-1:run: String formatting operations should always use a tuplefor non-mapping values
************* Module twisted.scripts.test.test_tap2deb
W9208: 19,0:TestTap2DEB: Missing docstring
W9208: 23,4:TestTap2DEB.setUp: Missing docstring
W9012: 26,0: Expected 2 blank lines, found 1
W9402: 71,0: The first letter of comment should be capitalized
C0103: 73,8:TestTap2DEB.test_basicOperation: Invalid name "tap_name" (should match ([a-z_][a-zA-Z0-9]*)$)
W9501: 74,33:TestTap2DEB.test_basicOperation: String formatting operations should always use a tuplefor non-mapping values
C0103: 74,8:TestTap2DEB.test_basicOperation: Invalid name "tap_file" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103: 76,8:TestTap2DEB.test_basicOperation: Invalid name "build_dir" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103: 77,8:TestTap2DEB.test_basicOperation: Invalid name "input_dir" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103: 78,8:TestTap2DEB.test_basicOperation: Invalid name "input_name" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103: 88,8:TestTap2DEB.test_basicOperation: Invalid name "debian_dir" (should match ([a-z_][a-zA-Z0-9]*)$)
  1. The _checkForDebBuild method is not really useful, you can do the work in setUp directly.
  1. Test docstrings generally shouldn't contain "should".
  1. As you replaced the os.popen('822-date'), you may as well use the datetime module.

Base on the ticket title I would have been inclined to close this ticket as wontfix, but as a test improvement it looks nice. Please resubmit for review once fixed.

comment:17 Changed 4 years ago by Thijs Triemstra

(In [38445]) address review comments, refs #5001

comment:18 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Thanks for your review. Addressed the comments and forced a new build.

comment:19 Changed 4 years ago by lvh

Owner: set to lvh

comment:20 Changed 4 years ago by lvh

Keywords: review removed
Owner: changed from lvh to Thijs Triemstra

Hi thijs,

Looking at the build there seems to be some remaining warts.

pyflakes failures:

twisted/scripts/tap2deb.py:59: local variable 'maintainer' is assigned to but never used
twisted/scripts/tap2deb.py:60: local variable 'description' is assigned to but never used
twisted/scripts/tap2deb.py:62: local variable 'long_description' is assigned to but never used
twisted/scripts/tap2deb.py:63: local variable 'twistd_option' is assigned to but never used
twisted/scripts/tap2deb.py:64: local variable 'date' is assigned to but never used
twisted/scripts/tap2deb.py:66: local variable 'python_version' is assigned to but never used

twistedchecker errors:

************* Module twisted.scripts.tap2deb
W9208:  1,0: Missing docstring
W9208: 15,0:MyOptions: Missing docstring
C0301: 18,0: Line too long (98/79)
C0301: 25,0: Line too long (114/79)
C0103: 38,0: Invalid name "type_dict" (should match (([A-Z_][A-Z0-9_]*)|(__.*__)|([a-z_][a-zA-Z0-9]+)|([A-Z][a-zA-Z0-9]+))$)
W9208: 47,0:run: Missing docstring
C0103: 54,4:run: Invalid name "tap_file" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103: 55,4:run: Invalid name "base_tap_file" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103: 57,4:run: Invalid name "deb_file" (should match ([a-z_][a-zA-Z0-9]*)$)
C0301: 60,0: Line too long (87/79)
C0103: 62,4:run: Invalid name "long_description" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103: 63,4:run: Invalid name "twistd_option" (should match ([a-z_][a-zA-Z0-9]*)$)
W9501: 66,21:run: String formatting operations should always use a tuplefor non-mapping values
C0103: 66,4:run: Invalid name "python_version" (should match ([a-z_][a-zA-Z0-9]*)$)
W9401: 96,0: Comments should begin with one whitespace
W9010:120,0: Trailing whitespace found in the end of line
C0301:140,0: Line too long (84/79)
W9501:200,-1:run: String formatting operations should always use a tuplefor non-mapping values

Besides the obvious ones (lines too long, missing docstrings...), the pyflakes issues seem to come from using % vars() instead of specifying interpolation variables explicitly.

I'm hoping that I'm not looking at the wrong file, because e.g.

C0103: 66,4:run: Invalid name "python_version" (should match ([a-z_][a-zA-Z0-9]*)$)

... was already mentioned by therve in his last review.

cheers and thanks again for working on this

lvh

comment:21 Changed 4 years ago by Thijs Triemstra

(In [38655]) address review comments, refs #5001

comment:22 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Thanks for your review, lvh. I fixed the obvious warnings, but left the names for vars() the same to prevent breaking more..

Forced a new build.

comment:23 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra
  1. At least two of the tests don't need to be skipped, if dpkg-buildpkg isn't installed.
  2. from email.utils import formatdate as now ?

    As you replaced the os.popen('822-date'), you may as well use the datetime module.

  1. def run(options=None):My first guess would be that options would be an instance of usage.Options. Perhaps args or argv? It should also have @param.
  2. self.assertEqual(len(inputDir.listdir()), 4) seems like something that it would make sense to specify explicitly. And the same for the output (I realize there may be some arch dependence in there, although maybe it should be all?)
  3. There are a couple of changes to the created files. I assume this is due to changes in the underlying tool. Does this mean we don't support older tools now? And if so, what versions.
  4. Since we are using subprocess now, is there any reason to change the directory of the python process?
  5. Since you are touching almost all of the code here, can you update change the variables to match twisted's naming convention.
  6. The docstring for run should explain what the script does: it is the script, it doesn't run it.

Please resubmit for review after addressing the above issues.

comment:24 Changed 4 years ago by Thijs Triemstra

(In [38976]) address review comments, refs #5001

comment:25 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Thanks for your review.

Replying to tom.prince:

  1. At least two of the tests don't need to be skipped, if dpkg-buildpkg isn't installed.

Fixed.

  1. from email.utils import formatdate as now ?

    As you replaced the os.popen('822-date'), you may as well use the datetime module.

I could use datetime but it would require more code (unless im missing something):

>>> t = datetime.datetime.utcnow()
>>> now = t.strftime('%a, %d %b %Y %H:%M:%S %z')
'Sat, 29 Jun 2013 10:39:24 '

so why use datetime here?

  1. def run(options=None):My first guess would be that options would be an instance of usage.Options. Perhaps args or argv? It should also have @param.

Fixed. Also added an additional test for the required maintainer argument.

  1. self.assertEqual(len(inputDir.listdir()), 4) seems like something that it would make sense to specify explicitly. And the same for the output (I realize there may be some arch dependence in there, although maybe it should be all?)

Fixed.

  1. There are a couple of changes to the created files. I assume this is due to changes in the underlying tool. Does this mean we don't support older tools now? And if so, what versions.

I've added the following extra files to the output deb file:

  • debianDir.child('compat'). This was to get rid of this warning (and build also fails for me otherwise):
    dh_clean
    dh_clean: No compatibility level specified in debian/compat
    dh_clean: This package will soon FTBFS; time to fix it!
    
  • debianDir.child('source').child('format'). This seems to be part of a .deb nowadays, but without it tap2deb still functions so I removed it again.

Using dpkg-buildpackage on Ubuntu 12.04:

$ dpkg-buildpackage --version
Debian dpkg-buildpackage version 1.16.1.2.

So without the first extra file the tool doesn't run here (and tests will fail). Does it need a .bugfix file instead?

  1. Since we are using subprocess now, is there any reason to change the directory of the python process?

tap2deb always builds the package in '.build' of the current work directory, so without this change it would build in _trial_temp but we want it to build in _trial_temp/twisted.scripts.test.test_tap2de/TestTap2DEB/test_basicOperation/OruqLn/temp/.build/.

  1. Since you are touching almost all of the code here, can you update change the variables to match twisted's naming convention.

Fixed.

  1. The docstring for run should explain what the script does: it is the script, it doesn't run it.

Fixed.

Forced a new build.

comment:26 Changed 4 years ago by Tom Prince

  1. Since we are using subprocess now, is there any reason to change the directory of the python process?

tap2deb always builds the package in '.build' of the current work directory, so without this change it would build in _trial_temp but we want it to build in _trial_temp/twisted.scripts.test.test_tap2de/TestTap2DEB/test_basicOperation/OruqLn/temp/.build/.

I was obliquely suggesting changing the working directory of the subprocess without changing the working directory of the trial process.

comment:27 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra

I get the following when trying to run the test on debian squeeze.

[FAIL]
Traceback (most recent call last):
  File "/home/tomprince/src/twisted/twisted/scripts/test/test_tap2deb.py", line 91, in test_basicOperation
    ['build-stamp', 'debian', 'install-stamp', 'lemon.tap'])
  File "/home/tomprince/src/twisted/twisted/trial/unittest.py", line 270, in assertEqual
    % (msg, pformat(first), pformat(second)))
twisted.trial.unittest.FailTest: not equal:
a = ['debian', 'lemon.tap']
b = ['build-stamp', 'debian', 'install-stamp', 'lemon.tap']

I'd be happier if this ticket didn't try to adjust the behavior of tap2deb to support newer versions of the dpkg tools, and another ticket was filed to deal with those changes.

Thanks for working on this. Please resumbit for review, after

  1. Reverting the changes to support newer packaging tools.
  2. Changing the tests to not change the directory of the trial process.

comment:28 Changed 4 years ago by Thijs Triemstra

(In [40033]) address review comments, refs #5001

comment:29 in reply to:  27 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Thanks for the review.

Replying to tom.prince:

  1. Reverting the changes to support newer packaging tools.

There was one additional 'compat' change I previously made that is reverted now, tests still pass here.

  1. Changing the tests to not change the directory of the trial process.

Removed the os.chdir calls that were unnecessary.

Forced a build, up for review again.

comment:30 Changed 3 years ago by Richard Wall

Keywords: review removed
Owner: set to Thijs Triemstra

Thanks Thijs,

Notes:

Points:

  1. Twistedchecker warning
    1. http://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1305/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
      ************* Module twisted.scripts.tap2deb
      C0301:130,0: Line too long (86/79)
      
    2. There's also another long line in tap2deb which isn't reported by tc.
  1. Python3 syntax errors
    (Twisted3)[richard@zorin trunk]$ ./admin/run-python3-tests twisted.scripts.test.test_tap2deb
    Traceback (most recent call last):
      File "./admin/run-python3-tests", line 30, in <module>
        unittest.main(module=None, argv=["run-python3-tests", "-v"] + testModules)
      File "/usr/lib64/python3.3/unittest/main.py", line 124, in __init__
        self.parseArgs(argv)
      File "/usr/lib64/python3.3/unittest/main.py", line 168, in parseArgs
        self.createTests()
      File "/usr/lib64/python3.3/unittest/main.py", line 175, in createTests
        self.module)
      File "/usr/lib64/python3.3/unittest/loader.py", line 137, in loadTestsFromNames
        suites = [self.loadTestsFromName(name, module) for name in names]
      File "/usr/lib64/python3.3/unittest/loader.py", line 137, in <listcomp>
        suites = [self.loadTestsFromName(name, module) for name in names]
      File "/usr/lib64/python3.3/unittest/loader.py", line 96, in loadTestsFromName
        module = __import__('.'.join(parts_copy))
      File "/home/richard/projects/Twisted/trunk/twisted/scripts/test/test_tap2deb.py", line 8, in <module>
        from twisted.scripts import tap2deb
      File "/home/richard/projects/Twisted/trunk/twisted/scripts/tap2deb.py", line 163
        debianDir.child('init.d').chmod(0755)
                                           ^
    SyntaxError: invalid token
    
    1. Consider fixing that (since you've fixed other Python3 incompatibilities in this branch). But on the other hand, the diff is already big, so it may not worth fixing that here.
  1. There are one or two bits of ugly formatting / indentation. Consider changing those:
    1. {or\
    2. % vars()
    3. {{{debianDir.child('README.Debian').setContent( This package was auto-generated by tap2deb\n)}}}
    4. {{{error = self.assertRaises(SystemExit, tap2deb.run,

["--tapfile", "foo"])}}}

  1. I searched for other tap2deb tickets and many of the comments seem to suggest that it should be abandoned. I agree, I can't imagine using tap2deb for deploying twisted applications these days. But I can't actually find a deprecation ticket. Shall we create such a ticket?
    1. https://twistedmatrix.com/trac/ticket/4426#comment:2
    2. https://twistedmatrix.com/trac/ticket/546#comment:3
    3. https://twistedmatrix.com/trac/ticket/968#comment:5
  2. It seems slightly crazy that we've spent so much time on a tool which is probably unused and no longer useful (see irc conversation below)

But anyway, I think the changes look fine and the new test coverage is a good improvement.

It's been reviewed 6 times by 4 different reviewers.

So please merge after at least fixing the long lines, merging forward and checking for clean build results.

-RichardW.

Conversation about tap2deb on #twisted-dev

14:01 < rwall> And I'm reviewing #5001 which seems like a lot of unnecessary work on an obsolete tool.
14:01 < hynek> rwall: i recall it not being flexible enough for my requirement and IIRC worked under sub-optimal assumptions
14:02 < rwall> Ok, so you might you might have used it. It might be worth maintaining?
14:02 < exarkun> It shouldn't be part of Twisted.
14:02 < hynek> rwall: it’s too long ago but i recall my reaction was more or less “ew” :)
14:03 < rwall> That's what various comments seem to suggest, but I can't find any ticket to actually deprecate it.
14:03 < exarkun> It should be part of Debian.  Since that is probably impossible, it should at least be third-party where it can be kept up to date
                 with Debian Python policy.
14:03 < exarkun> The problem is that any time anyone suggests removing it some random person shows up and says "but!!!"
14:03 < rwall> ok
14:03 < exarkun> (ditto tap2rpm)
14:03 < rwall> yep
14:03 < hynek> loudly deprecate and let rot seems most appropriate.
14:04 < exarkun> I don't want it to rot.  I want it to be a great, working tool.
14:04 < hynek> deploying is too individual IMHO to write a single tool
14:04 < exarkun> It's *already* rotten because no one is willing to maintain it as *part of* Twisted.
14:04 < exarkun> hynek: Who says you have to have a single tool?
14:05 < exarkun> hynek: The solution to "single tool can't do everything" is not "have zero tools" - it's "have many tools".
14:06 < rwall> How does it relate to the debian packaging stuff in setuptools? Isn't that best route?
14:06 < hynek> it would be probably more helpful to properly document how to do it with existing tools like FPM
14:06 < exarkun> rwall: tap2{deb,rpm} are configuration packaging tools
14:07 < exarkun> hynek: If FPM is a pure superset of the functionality, great!  Then tap2{deb,rpm} are redundant and we don't need them.
14:07 < exarkun> I don't know anything about FPM and I'm not likely anytime soon.  Do you think you could write up how FPM replaces these tools?
14:07 < hynek> depending on the aspect, fpm is both a superset and subset :)
14:07 < exarkun> Then I'd be happy to just deprecate them and not bother turning them into third-party projects.
14:08 < hynek> this is not a Twisted problem at all. we’re talking about deploying a python application whose command name is incidentally “twistd”
14:09 < exarkun> hynek: Historically that is an entirely uncompelling argument given the total failure of the Python community to produce deployment
                 tools.
14:09 < exarkun> hynek: I'm happy to be convinced that this is no longer the case - but please don't try to convince me with an IRC conversation.
                 Write something persistent.
14:10 < hynek> to be fair, other communities don’t do better at all. :)
14:10 < exarkun> Or if something is written already, then just give me a link.
14:10 < rwall> exarkun, hynek: ok, I may have opened a can of worms. Thanks for the comments. I'll paste them into the ticket.
14:17 < itamar> all tap2deb does is create a init.d script that starts/stops twistd with some arguments you give it
14:18 < itamar> one hopes that systemd/upstart/whatever make that so easy that having a tool for it is redundant
14:19 < hawkowl> they make it easy, but now you need a tool for making upstart scripts for ubuntu, sysvinit scripts for debian and systemd scripts
                 for god knows what :P
14:21 < rwall> itamar: Oh. So you'd need to create a separate package containing the code anyway.
14:22 < rwall> And come to think of it, the howto spells that out
14:22 < rwall> "Note: tap2rpm and tap2deb do not package your entire application"
14:22 < hynek> well.
14:22 < rwall> https://twistedmatrix.com/documents/current/core/howto/basics.html#auto2
14:23 < hynek> you can use like 5 process managers on debian alone. and it’s fairly trivial nowadays too – the days of SystemV scripts are long
               forgone

comment:31 Changed 3 years ago by Thijs Triemstra

Branch: branches/str-methods-scripts-5001-2branches/str-methods-scripts-5001-3

(In [40139]) Branching to 'str-methods-scripts-5001-3'

comment:32 Changed 3 years ago by Thijs Triemstra

Status: newassigned

Thanks for the review rwall. I'm going to merge it when the build is green, this ticket escalated already it feels like and if the consensus is to deprecate the tool, then I don't want to waste any more time on it.

comment:33 Changed 3 years ago by Thijs Triemstra

Resolution: fixed
Status: assignedclosed

(In [40141]) Merge str-methods-scripts-5001-3: Replace deprecated string.strip() and add basic tests for twisted.scripts.tap2deb.

Author: thijs, nueces Reviewer: lvh, exarkun, therve, tom.prince, rwall Fixes: #5001

Note: See TracTickets for help on using tickets.