Opened 16 months ago

Closed 7 weeks ago

#7035 enhancement closed fixed (fixed)

Port the release scripts to use Git instead of Subversion

Reported by: multani Owned by: hawkowl
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/git-release-7035
(diff, github, buildbot, log)
Author: hawkowl Launchpad Bug:

Description

The release scripts are made to use the source repository currently by Twisted, which is Subversion.

If Twisted's repository is to be changed from Subversion to Git, these scripts have to be updated.

Since tagging with Subversion and Git is a bit different, it has yet to be seen if these changes could work even if there's currently no repository to push the Git modifications on (the Git mirror on Github is supposed to be this, a mirror of the Subversion repository).

Related documentation can be found at Infrastructure/SwitchToGit

Attachments (3)

7035-release-scripts-use-git-1.patch (11.3 KB) - added by multani 16 months ago.
7035-release-scripts-use-git-2.patch (11.4 KB) - added by multani 16 months ago.
Forgot the topfile
7035-release-scripts-use-git-3.patch (11.5 KB) - added by multani 16 months ago.
Slightly better version than -2, fix wording, add comments, remove useless clone+init sequence in tests

Download all attachments as: .zip

Change History (23)

Changed 16 months ago by multani

Changed 16 months ago by multani

Forgot the topfile

comment:1 Changed 16 months ago by multani

  • Keywords review added

This patch ports the release scripts to use Git instead of Subversion.

It's probably not finished yet, there might be things I missed, but all the code and the tests have been ported to Git.
One thing which is a bit different between both systems, is that Git has to know where the root of the repository is so it can work inside - that's what the -C flag is for that I had to add. I didn't use --git-dir since it's less practical:

  • the former just say to Git "here's your working directory where you should do your job from";
  • whereas the second one says "here is the .git that corresponds to the repository you should work on. (Note that setting this doesn't work if there is a .git already present in the working directory the Git is launched from, so that's even less practical.)

It's up for review, I would like to have feedback on this, but it's probably not to be merged *as soon as the review is OK* if the rest of the process hasn't decided to move on to Git.

Changed 16 months ago by multani

Slightly better version than -2, fix wording, add comments, remove useless clone+init sequence in tests

comment:2 Changed 16 months ago by multani

comment:3 Changed 16 months ago by adiroiban

  • Keywords review removed
  • Owner set to multani

Thanks for your work on this.

I don't know much about release management, but I think that this patch introduces a break/disturbance in the release process.

What if this path is merged, but final migration is not done before the next release?

Instead of removing all SVN code, maybe the release command could have an extra flag, which trigger the version system in use... and default to SVN.

After final migration is done, a ticket can be created to remove SVN code.

In this way, we can have this code merged and used for testing/trying the whole Git thing, even if complete migration is not done.


I saw that you have updated to docstrings from SVN to Git. Maybe then can be updated to use VCS independent word so that next
time when a swich is done, they will not require an update.

---

Based of above commment, maybe instead of gitCommit and svnCommit we can have the method named commit.

Please let me know if you need any help, as I would like to help as much as possible with this migration.

Thanks!

comment:4 Changed 16 months ago by multani

adiroiban > I pushed some more modifications in the branch https://github.com/multani/twisted/tree/7035-release-scripts-use-git
I won't have much time to work on this in the next couple of months, so if you are interested to continue the work here, feel free (you can start from my branch, or do something else.)

I got advice on IRC to extend the scripts so it automatically detects which repository it is ran from..

comment:5 Changed 8 weeks ago by hawkowl

  • Author set to hawkowl
  • Branch set to branches/git-release-7035

(In [44768]) Branching to git-release-7035.

comment:6 Changed 8 weeks ago by hawkowl

  • Keywords review added; git removed
  • Owner multani deleted

I branched off multani's work and finished it up.

Bots are green, except for the ones still catching up, which *will* be green.

Please review!

comment:7 Changed 8 weeks ago by moshez

  • Keywords review removed
  • Owner set to hawkowl

Hi HawkOwl, thanks for working on this!

Here are some things that I think should be addressed:

  1. In "getRepositoryCommand", I think it would make more sense to have the commands decide whether they like it, rather than duplicating the functionality. I.e., use "ensureWorkingDirectory". This would also cut down on some code redundancy.
  2. Currently, problems with svn or git not being installed or being the wrong version are lumped together with working directory issues. It would make sense to also have "ensureInstalled", running git/svn --version and checking the output. (Essentially, promote the gitskip/svnskip from tests to production)
  3. In makeTempDir, why not use the Python facilities for making temporary files with an explicit "dir="?
  4. If NewsBuilderMixin.setUp, if createStructure() fails, the tearDown won't be called and we won't clean up the tempfiles. Should we use addCleanup?
  5. In BuildAllTarballsTests, we seem to no longer be testing SVN? This seems inconsistent with the rest of the methodology here.
  6. There are a bunch of places (e.g., RepositoryCommandDetectionTest) where I see no functionality to remove the temporary directories.

Please address and resubmit.

In addition, here are some ideas you may want to consider:

  1. Maybe the "Command" should be an explicit zope.interface? This would make it clearer what is part of the command interface. (Especially if we add helper methods later).

comment:8 Changed 8 weeks ago by hawkowl

  • Keywords review added

Hi moshez, thanks for the review!

  • Done.
  • I'm not sure about this, since release management isn't done on 12.04 ;)
  • Done.
  • I've made a TestCase that does an addCleanup, which also fixes no. 6 -- all the temp files should be cleaned up.
  • The BuildAllTarballs tests now do git + svn.

I'm not sure if the z.i will get us much, though.

Builders spun, please review.

comment:9 Changed 8 weeks ago by moshez

  • Keywords review removed

Hi hawkowl,

Thanks for working on this. I think this is OK to merge if you want to. Just responding to some of the points you decided to pass on --

  1. Assuming what version people try to do RM from seems scary, especially if a new RM gets obtuse errors. I agree it's probably not super-important, though.
  2. What z.i would give you would be not having lines like "@rtype: L{SVNCommand} or L{GitCommand}" :)

As I said, neither of this are show-stoppers -- go ahead and merge if you want to (or change and resubmit if you want to do that...)

comment:10 follow-up: Changed 8 weeks ago by adiroiban

I guess that in the near future SVNCommand will be removed and we will have an interface implemented by a single class.

I am not sure how the release is done now, but I guess that a good way to do it is to have a dedicated system/VM/container from which the release command are executed.
In this way you don't have to worry about support WIndows/Linux/OSX or various versions of Linux / OSX.

Thanks for the work on this both the patch and the review!

comment:11 in reply to: ↑ 10 Changed 8 weeks ago by moshez

Replying to adiroiban:

I guess that in the near future SVNCommand will be removed and we will have an interface implemented by a single class.

That depends on your definition of "near", probably. I'm guessing that in another 10 years, we'll want to move to another version control system, and it'll be super-useful to have an interface to guide this port. In the next couple of weeks, we'll need both SVN and Git in the code base. Somewhere in between, I suspect you're right.

I am not sure how the release is done now, but I guess that a good way to do it is to have a dedicated system/VM/container from which the release command are executed.

I think the end-game here is to have a "push button" release. But regardless, verifying that the VM/container/whatever has Git/SVN installed correctly in the Python code is probably not a bad thing. But I don't think it's a show-stopper thing, either way.

comment:12 Changed 8 weeks ago by hawkowl

  • Keywords review added

I fixed up the z.i stuff, made them all static methods (since there's no reason to have an instance) and cleaned up a few things.

Putting up for review again.

comment:13 Changed 8 weeks ago by hawkowl

  • Owner hawkowl deleted

comment:14 Changed 8 weeks ago by hawkowl

(builders spun and green, also)

comment:15 Changed 7 weeks ago by adiroiban

  • Keywords review removed
  • Owner set to hawkowl

Changes look good. Thanks.

Maybe instead of "No supported VCS can be found in %s" we can have "No supported VCS can be found for %s"

From test I see that the code need git version 2 or never but the runtime code raise no warning to inform that git is not found or is to old.

Since this is a dev only code, I think that we can merge it as it is now and have a separate ticket once a developer needs more feature from this code.

Thanks!

comment:16 Changed 7 weeks ago by hawkowl

  • Resolution set to fixed
  • Status changed from new to closed

(In [44886]) Merge git-release-7035: Port the release scripts to use Git and Subversion

Author: multani, hawkowl
Reviewers: adiroiban, moshez
Fixes: #7035

comment:17 Changed 7 weeks ago by hawkowl

  • Resolution fixed deleted
  • Status changed from closed to reopened

(In [44892]) Revert r44886: Port the release scripts to use Git and Subversion

This breaks on Windows! Windows doesn't have git, and it got caught in the "oh there's one failure on Windows". Whoops.

Reopens: #7035

comment:18 Changed 7 weeks ago by hawkowl

  • Keywords review added
  • Owner hawkowl deleted
  • Status changed from reopened to new

I think this should be fine now, please review.

Builders spun.

comment:19 Changed 7 weeks ago by adiroiban

  • Keywords review removed
  • Owner set to hawkowl

looks good. ... twistedchecker is red

if you are happy with the changes please merge.

thanks!

comment:20 Changed 7 weeks ago by hawkowl

  • Resolution set to fixed
  • Status changed from new to closed

(In [44897]) Merge git-release-7035: Port the release scripts to use Git and Subversion

Author: multani, hawkowl
Reviewers: adiroiban, moshez
Fixes: #7035

Note: See TracTickets for help on using tickets.