Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#8491 enhancement closed fixed (fixed)

Convert scripts in bin to console_scripts

Reported by: Craig Rodrigues Owned by: GitHub <noreply@…>
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: 8491-rodrigc-console
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description

The following scripts in bin should be converted to console_scripts:

  • ckeygen
  • pyhtmlizer
  • mailmail
  • tkconch
  • trial
  • cftp
  • conch
  • twistd

Change History (22)

comment:1 Changed 6 years ago by Craig Rodrigues

Branch: 8491-rodrigc-console
Keywords: review added

comment:2 Changed 6 years ago by hawkowl

The only thing: trial being a console script means that it's impossible to import it and test a Twisted that is not installed.

comment:3 Changed 6 years ago by hawkowl

(that is, bin/ might have to remain, but we still install twistd -- then to test a checkout you use bin/twistd as usual)

comment:4 Changed 6 years ago by Craig Rodrigues

When you run the tests under tox, the tox command will:

  1. run setup.py and installs into the build directory
  2. run trial from inside the build/whatever/bin directory

For example from the top-level directory of a twisted checkout, you can do:

tox -e py27-tests-posix twisted/test/test_nmea.py

and it will do the right thing

So, you don't need the scripts in bin/ to run the tests.

comment:5 Changed 6 years ago by Adi Roiban

Keywords: review removed
Owner: set to Craig Rodrigues

Changes look good, with the exception of sub-optimal diff coverage about 30%... and some failing builders.

Happy to see that we got rid of this boilerplate code.

Please write tests for the new code.

We will need to update the builders to always use tox for setting up the environment and dependencies and running the test.

I see that the test nomodules is failing... and so does the windows builder.

We need to have all builders pass (with the exception of twistedchecker) before landing this. I can take care of that.. as soon as the current buildbot changes from the review queue are cleared.

Very minor comment. In getConsoleScripts I suggest to start with a list containing py2 and py3 scripts and if we are not on py3 to also extend the list with the py2 only scripts. In this way we should reduce duplication.

Please check that the manifest test is passing and add tests so that we have 100% coverage for the changes.

Thanks!

comment:6 Changed 6 years ago by Craig Rodrigues

The builder scripts will need to be modified slightly. Luckily it is not too hard. The easiest thing is to run things under tox.

However, another easy modification can be done. In the root directory of the twisted folder, we can have this logic:

import os
import subprocess

if not os.path.exists("bin/trial"):
    subprocess.call(["python", "setup.py", "develop", "--script-dir", "bin"])

That's the easiest way to get the builders working with the new console_scripts.

comment:7 Changed 6 years ago by Craig Rodrigues

Keywords: review added
Owner: Craig Rodrigues deleted

I updated the patch, and now the check-manifest errors are gone.

I also submitted this patch for the buildbots: https://github.com/twisted-infra/braid/pull/210

I'm new to the buildbots so don't know if that is the exact place where the change is needed, but it seemed like it.

comment:8 Changed 6 years ago by Adi Roiban

Thanks for the update. I will look at fixing the buildbot.

comment:9 Changed 6 years ago by Adi Roiban

See #8544 first so that we can update the buildslave to always use tox

comment:10 Changed 6 years ago by hawkowl

Just a passing note: the topfile will be mangled when the docs are built, as newlines are removed. We don't need to mention stuff about the bin/ dir to users in the topfile, so just make it a .feature that says that the scripts like trial and twistd have been updated to be setuptools console scripts.

comment:11 Changed 6 years ago by Craig Rodrigues

OK, I changed the topfile as per the feedback.

comment:12 Changed 6 years ago by Adi Roiban

Keywords: review removed
Owner: set to Craig Rodrigues

Thanks for the branch.

So this branch does 3 main things:

  1. Converts all bin/* to entry_points. As per the ticket description.
  2. removes bin/trial so we can no longer run twisted from the local dir but alwasy run the installed version
  3. merge setup.py and setup3.py.

To move forward I think that we should keep bin/trial but don't install it with setup.py . bin/trial can be killed once we move to src/twisted ... Amber is working on this move. ... or in a separate ticket in which we update the documentation for the development process.

Do the setup.py merge in a separate ticket

Please check my comment and resubmit.

Thanks!

comment:13 Changed 6 years ago by Craig Rodrigues

Owner: Craig Rodrigues deleted

Feedback incorporated

comment:14 Changed 6 years ago by Adi Roiban

Thanks!

The commit history looks strange... I remember that I saw a fix for the topfile.

There is also a comment from Alex regarding extra indentation.

Is is really needed to rewrite the history ? :) If history is not changed, GitHub PR will give me a nice link with the diff of the changes since my last view of the PR.


Do we really need the getConsoleScripts script. Can we just put the list in setup.py ? From what I can see the list is very much declarative... and if we have to duplicate that info, I think that we can live with a bit of duplication.

The current GetConsoleScriptsTests seems useless... if we port a script to python3 but we forget to add it ot the list, the test will still pass... also, if we introduce a new script for py2 it will fail with an error like 9 is not 8 ... which is not really helpful.

comment:15 Changed 6 years ago by Craig Rodrigues

I think that having any test for getConsoleScripts() is a waste of time, but before when you set the codecov requirement to require 100% code coverage, I had to do something. To me, that is more a limit of how you have implemented codecov, since you do not run the setup.py script itself under coverage.

I think what I have submitted is good, and does the job. Are you going to allow this review to move forward, so I can commit it and proceed to my next submissions of patches, or are you going to block this?

comment:16 Changed 6 years ago by Craig Rodrigues

Keywords: review added

comment:17 Changed 6 years ago by Adi Roiban

Keywords: review removed
Owner: set to Craig Rodrigues

I don't want to block this... but I think that writing a useless test just to please codecov is wrong.

We should not allow our tools to force us to do stupid things... the tools are designed to prevent us from doing stupid things.... so this is backwards :)

If you think that the test is useless don't write it.

The idea is that in the future, another developer implementing a script might see the test failing, will take a look at it and just like you, will think that the test is useless and will remove it.

From what I can see, the problem here is that we don't have tests for the setup.py itself... so instead of writing useless tests for dist.py we should consider writing a test for setup.py

Now I don't know why we don't have tests for setup.py itself.

It should not be hard to rewrite it as the example from here https://gist.github.com/adiroiban/5b461f3ca96828a3036c99964d1d236e and then write tests for the final arguments passed to setup

I have created #8606 as a followup

So, please fix the release notes, remove the useless test and merge.

Many, many thanks for your work on this issue!

comment:18 Changed 6 years ago by Craig Rodrigues

Owner: Craig Rodrigues deleted

I don't want to comment on the stupidity of the tools, because behind the tools is the person who is implementing the tools.

I'm just a contributor who is jumping through hoops to pass the review process, and the hurdles that you keep throwing to block getting commits into Twisted.

You are the one who is implementing integration with codecov, and implementing the hooks to block things. I suggest that you run setup.py under coverage itself, so that changes to setup.py will be under coverage.

comment:19 Changed 6 years ago by Adi Roiban

I don't know how you want me to run setup.py under coverage itself. I am happy to review such a change. This is why I have created #8606

You can hit me hard, I will not take it personally :)

No news is good news, and if nobody is complaining is hard to tell what is wrong or what should be changed or improved.

I know that codecov/patch status is stupid ... this is why I have disabled it and this is why we had the conversation over the mailing list.

I know that sometimes I do stupid things and I don't think twice ... maybe a bit to often.

I know that I am a PITA and I am sorry about that... I am working to improve that, but please hit me hard with feedback as it will help me to be less of a PITA.

I wish there we would have many other people doing reviews so if I am asking something stupid during a review, another peer can step up, pinch me to wake up and get back on the right track :)

If you don't agree with my feedback, feel free to put the ticket back for review, I will not take it personally, and basically I don't care :)

comment:20 Changed 6 years ago by GitHub <noreply@…>

Owner: set to GitHub <noreply@…>
Resolution: fixed
Status: newclosed

In c5a4c635:

Error: Processor CommitTicketReference failed
 does not appear to be a Git repository. See the log for more information.

comment:21 Changed 6 years ago by Adi Roiban

I would still like to have commit messages based on the current documented format.

If you don't like the current format for the commit message, please send a message to the mailing list.

The current format ask for the first line to contain a short description of the change... and the default GitHub commits don't have this information.

comment:22 Changed 6 years ago by Craig Rodrigues

I see that Glyph has used a similar format:

https://github.com/twisted/twisted/commit/e309d6309504ebb859e77d6c5ee3df6144224dc4 https://github.com/twisted/twisted/commit/1a0e1d1adcaa3913704b9868d80d1a15bb41dfcf

I believe that this format is acceptable for Twisted, and I feel comfortable using this format for merge commits in Twisted.

Regarding coverage of setup.py, you added the commands in tox.ini to invoke coverage, so I fail to see why you cannot add commands in tox.ini to invoke setup.py under coverage.

Note: See TracTickets for help on using tickets.