Opened 3 years ago

Closed 3 years ago

#8214 enhancement closed fixed (fixed)

Twisted should move to a src/ directory layout

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: src-move-8214-5
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl

Description (last modified by hawkowl)

This is current best practice (https://hynek.me/articles/testing-packaging/) and fixes quite a few issues:

  • tox having to cd to a different directory to run the tests without running against the checkout accidentally (all of the toxrundir lines)
  • reduce confusion as scripts will put stuff in the local checkout dir, instead of sometimes random dirs in build/ (like .coverage files, or apidocs)
  • allow us to have different packages in the repo that work in lockstep (see https://twistedmatrix.com/trac/ticket/7945)

Change History (26)

comment:1 Changed 3 years ago by hawkowl

Author: hawkowl
Branch: branches/src-move-8214

(In [46887]) Branching to src-move-8214.

comment:2 Changed 3 years ago by hawkowl

This depends on https://twistedmatrix.com/trac/ticket/7985 to make the py3 tests pass on an installed Twisted.

comment:3 Changed 3 years ago by hawkowl

Branch: branches/src-move-8214branches/src-move-8214-2

(In [46986]) Branching to src-move-8214-2.

comment:4 Changed 3 years ago by hawkowl

Branch: branches/src-move-8214-2branches/src-move-8214-3

(In [47107]) Branching to src-move-8214-3.

comment:5 Changed 3 years ago by hawkowl

Requirement for the buildbots to work with this patch: https://github.com/twisted-infra/braid/pull/191

comment:6 Changed 3 years ago by Thijs Triemstra

Will this retain the svn commit history? Isn't it better to svn mv the code, it now looks like you're doing a copy.

comment:7 Changed 3 years ago by hawkowl

It should -- but I'm using the git-svn bridge :(

I will have to try it using svn mv.

comment:8 Changed 3 years ago by hawkowl

Branch: branches/src-move-8214-3branches/src-move-8214-4

(In [47113]) Branching to src-move-8214-4.

comment:9 Changed 3 years ago by hawkowl

Nope, svn mv still breaks git-svn. :(

Looks like this'll have to wait until the git move.

comment:10 Changed 3 years ago by hawkowl

Owner: set to hawkowl

We're on Git now. Time to revisit this.

comment:11 Changed 3 years ago by hawkowl

Branch: branches/src-move-8214-4src-move-8214-5

comment:12 Changed 3 years ago by hawkowl

Keywords: review added

I am fairly happy with this, please review. Builders are spinning.

comment:13 Changed 3 years ago by hawkowl

Owner: hawkowl deleted

comment:15 Changed 3 years ago by Adi Roiban

Looks good, but what is the point of this move?

Why are you only fairly happy with this branch. Why is not right with this branch and stops you for being fully happy with it :)

It would help to understand what are the drawbacks that you have identified or the reasons why you are not very happy with the branch :)


The link to tox.ini line 13 gave me no clue about why is this change is needed :)

The bin/trial script is still importing the Twisted from the 'src/' folder. How is this helping with running tests against an installed Twisted version?

Leaving it for review so that another review can check it.

I don't see any harm in merging this branch, but in the same time I don't see the benefits.

Thanks!

comment:16 Changed 3 years ago by hawkowl

I'm 'fairly happy' in that the builds are green, but I am having to move every file in the repo :P

Line 13 is now https://github.com/twisted/twisted/blob/trunk/tox.ini#L16, the changedir hacks. It also helps with an installed version because the 'trial' (not 'bin/trial'!) you run in a virtualenv is now the installed twisted, not the random one on the path.

comment:17 Changed 3 years ago by Adi Roiban

Hm... but the changedir hack is still in the tox.ini file in the proposed branch... and I can see that bin/trial is still used ... but maybe this will be changes in a follow up ticket... but this is not clear to me and not follow up ticket is referenced here

Somehow I don't get it... but don't worry :) leaving this in the review queue so that another dev can take a look

comment:18 Changed 3 years ago by hawkowl

Right, those aren't needed, so I've removed them now. They were overriding the top-level one.

As far as using bin/trial, I've tried to remove that as well.

comment:19 Changed 3 years ago by Glyph

The github 'update branch' button didn't work, but I just merged this (there were no actual conflicts according to git) and pushed to it (236 commits behind!)

comment:20 Changed 3 years ago by Glyph

I added a little throwaway script to the branch which automates the process of doing the move of files from twisted/ to src/twisted/; this should help both with keeping it up to date with trunk and also helping deal with the post-merge fallout.

comment:21 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to hawkowl

Thanks hawkowl!

  1. As Adi pointed out, I think I know the motivation for doing this, but we should update the description of this ticket to clearly explain what the point is. Especially if you have a list of concrete problems that are caused by this. For example, I'm not sure I totally get why it will make removing that tox line easier...
  2. Things related to coverage XML generation seem to be failing. I don't know why those fail but other things pass, the errors I've seen are empty.

Hopefully the script I added will make it easier to keep it up to date as you poke at those.

comment:22 Changed 3 years ago by hawkowl

Description: modified (diff)

comment:23 Changed 3 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

I have managed to break the merge checker :D

I've updated the branch to match trunk, and also fixed up the description. Buildbot is green.

Please review.

comment:24 Changed 3 years ago by hawkowl

To check the diff: git fetch && git diff origin/trunk..origin/src-move-8214-5 --diff-filter=M

comment:25 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

All good. Thanks!

Let'd do it :)

comment:26 Changed 3 years ago by Amber Brown <hawkowl@…>

Resolution: fixed
Status: newclosed

In ed075da:

Merge src-move-8214-5: Move twisted under a src/ directory

Author: hawkowl, glyph
Reviewers: glyph, adiroiban
Fixes: #8214

Note: See TracTickets for help on using tickets.