Opened 8 months ago

Closed 3 months ago

#32121 closed enhancement (fixed)

Refactor some common configs and functions out of the git scripts.

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: git-scripts, 044-should
Cc: catalyst Actual Points: .5
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

There are a bunch of common configs and functions in the git scripts, we should find a nice way of refactoring them out.

Child Tickets

Change History (24)

comment:1 Changed 7 months ago by teor

Parent ID: #29603

comment:2 Changed 4 months ago by teor

Keywords: 044-should added
Milestone: Tor: unspecifiedTor: 0.4.4.x-final

When we deleted 0.2.9 and 0.4.0 from the git scripts, we made mistakes. (For example, #33217.)

Using a common config would make these changes easier to write and review.

comment:3 Changed 4 months ago by nickm

What's the right way to refactor this -- should we have a "tor-branches" script that the others source?

comment:4 Changed 4 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:5 in reply to:  3 Changed 4 months ago by dgoulet

Replying to nickm:

What's the right way to refactor this -- should we have a "tor-branches" script that the others source?

Yup I think that is what we need. A single file that has all our branches and we can ONLY update that when we have a new branch.

All our git scripts rely on a list of branches and they are all the same.

comment:6 Changed 4 months ago by nickm

Actual Points: .3
Status: acceptedneeds_review

Branch ticket32121 with PR at https://github.com/torproject/tor/pull/1720 .

I have tested this a little, but not extensively. I've done my best to write this in a way so that the individual scripts don't need to change how they use anything. This branch actually removes code from the scripts overall.

I'm afraid I have no idea how to write tests for this (#32122).

I'm not a very good bash programmer, so I might need your help in fixing up whatever issues you find.

comment:7 in reply to:  6 Changed 4 months ago by teor

Actual Points: .3.5

Replying to nickm:

Branch ticket32121 with PR at https://github.com/torproject/tor/pull/1720 .

I have tested this a little, but not extensively. I've done my best to write this in a way so that the individual scripts don't need to change how they use anything. This branch actually removes code from the scripts overall.

I really like this change, and I'm glad we've finally made it happen. The design is much better than what I had in mind.

I'm afraid I have no idea how to write tests for this (#32122).

Let's use the typical merge and test workflows, but without actually making any commits?

Here's a detailed design for some tests:

(We should probably make this test script into its own script, so we can run it in CI, and from the git hooks.)

Script arguments:

  • temporary TOR_FULL_GIT_PATH: mktemp -d by default, ${TRAVIS_BUILD_DIR}/git-scripts-test` in Travis

Useful TOR_EXTRA_CLONE_ARGS:

  • only clone recent commits: --depth ???
  • re-use existing git packs: --reference-if-able "$TOR_FULL_GIT_PATH/tor"

Setup:

# list the active tor branches, for testing and diagnostics
git-list-tor-branches.sh 

Help:

git-list-tor-branches.sh -h
git-setup-dirs.sh -h
git-pull-all.sh -h
git-merge-forward.sh -h
git-push-all.sh -h

Dry Run:

# now dry run all the scripts
git-setup-dirs.sh -n
git-pull-all.sh -n
git-merge-forward.sh -n
git-push-all.sh -n

List Branches:

git-list-tor-branches.sh 
git-list-tor-branches.sh -s
# Print and eval the bash code, but don't change the current shell's env
# branch_path
git-list-tor-branches.sh -b
echo $(eval "$(git-list-tor-branches.sh -b)"; echo "$WORKTREE")
# merge
git-list-tor-branches.sh -m
echo $(eval "$(git-list-tor-branches.sh -m)"; echo "$WORKTREE")

List maint-* and master branches:

git-list-tor-branches.sh -R
git-list-tor-branches.sh -s -R
# Print and eval the bash code, but don't change the current shell's env
# branch_path
git-list-tor-branches.sh -b -R
echo $(eval "$(git-list-tor-branches.sh -b -R)"; echo "$WORKTREE")
# merge
git-list-tor-branches.sh -m -R
echo $(eval "$(git-list-tor-branches.sh -m -R)"; echo "$WORKTREE")

Let's have a test script option that stops here: we can test some things in pre-commit and pre-push hooks (as long as we log output to a file), but cloning tor is too slow.

Merge:

# setup directories
git-setup-dirs.sh
# redundant pull, for testing, the merge script also does a pull before merging
git-pull-all.sh
# redundant merge forward, no changes, so git won't make merge commits
git-merge-forward.sh
# redundant push, no changes, so the script won't actually push anything
git-push-all.sh

Test Branch:

# add a fake remote
git remote add fake_ci_remote fake_ci_remote_url
# list the suffixes for the active tor branches, for testing and diagnostics
git-list-tor-branches.sh -s
# setup directories, use existing mode
git-setup-dirs.sh -u
# merge forward, test branch mode
git-merge-forward.sh -t fake_ci_test
# merge forward, use existing test branch mode
git-merge-forward.sh -t fake_ci_test -u
# redundant push, no changes, so the script won't actually push anything
git-push-all.sh -t fake_ci_test -r fake_ci_remote

Things we don't test:

# we can't test `git-push-all.sh -s`, because it actually pushes branches

I'll try to turn these notes into a script later today, after I've fixed some sponsor 55 bugs.

I'm not a very good bash programmer, so I might need your help in fixing up whatever issues you find.

shellcheck has made all of us better bash programmers :-)

comment:8 Changed 4 months ago by teor

(Didn't get to it today, found a bunch of bugs related to Sponsor 55. Feel free to have a go at testing!)

comment:9 in reply to:  8 Changed 4 months ago by nickm

Replying to teor:

(Didn't get to it today, found a bunch of bugs related to Sponsor 55. Feel free to have a go at testing!)

I probably won't have time for the tests today either, but the plan you outline above sounds pretty good and thorough to me.

comment:10 Changed 4 months ago by dgoulet

This is a really great change! And dynamic so we in theory don't have to change anything for each release. +1

comment:11 Changed 4 months ago by catalyst

Cc: catalyst added

comment:12 in reply to:  10 Changed 4 months ago by teor

Replying to dgoulet:

This is a really great change! And dynamic so we in theory don't have to change anything for each release. +1

We still have to update the list of branches in git-list-tor-branches.sh

But that's much better than changing different branch references in every script.

I also had some more ideas for testing:

We can test the pre-commit and parts of the pre-push script by making a scripted commit in the test repository, then pushing it to the repository that Travis automatically checks out. (Using a file path, so we know it will stay local.)

We can't test the upstream pre-push or the post-merge script, but that's ok.

comment:13 Changed 3 months ago by dgoulet

Reviewer: dgoulet

comment:14 Changed 3 months ago by nickm

FWIW, I still don't have time to write tests here, but I am running this code in production on my development environment, to try to shake out issues.

comment:15 Changed 3 months ago by dgoulet

I think that is good and fine to merge if you have been using it for a while. I mean, we are not talking about total destruction if something goes wrong. We can't remotely rewrite the history so nothing we can't undone.

comment:16 Changed 3 months ago by teor

I'd like to do some backports and test branches with this code, and then we can merge.

We can do the tests in #32122.

comment:17 Changed 3 months ago by dgoulet

Keywords: 043-backport added
Milestone: Tor: 0.4.4.x-finalTor: 0.4.3.x-final
Status: needs_reviewmerge_ready

Sooo I have found a typo I believe and instead of going back into another round of reviews, I just fixed it. The suffix had an extra r with I believe would be a typo? The -s results in:

_035
_r035
_041
_r041
_042
_r042
_043
_r043
_master

Here is the small fix:

-        suffix="_r${brname_nodots#release-}"
+        suffix="_${brname_nodots#release-}"

I've merged this upstream and the fix is an extra commit (a63b4148229ae8ce) so if by any chance it was on purpose, we can revert. It doesn't affect any of us until we update the merging scripts so at least there is an individual "revert path" we can do and work on any fixes.

Moving this to 043 for backport consideration as per teor's request. I will let you decide to which version you want it teor for backport.

comment:18 Changed 3 months ago by nickm

If you remove the r, then how can you tell the release-branch versions from the maint-branch versions?

comment:19 in reply to:  18 Changed 3 months ago by dgoulet

Replying to nickm:

If you remove the r, then how can you tell the release-branch versions from the maint-branch versions?

Oh... I thought that the suffix were just used to construct the array and that array specifically mentions the release vs maint? If not, lets revert.

comment:20 Changed 3 months ago by nickm

Yeah, let's remove that. Right now, -s is only used with -R, but it's not inherently necessary.

comment:21 Changed 3 months ago by nickm

(do you agree with merging without that change?)

comment:22 in reply to:  21 Changed 3 months ago by dgoulet

Replying to nickm:

(do you agree with merging without that change?)

To be clear, you mean reverting a63b4148229ae8ce ? If so, sure, if there is a use case, I'm fine with it.

comment:23 Changed 3 months ago by nickm

ok, reverted. Do we close this now and un-parent #32122 ?

comment:24 Changed 3 months ago by teor

Keywords: 043-backport removed
Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final
Resolution: fixed
Status: merge_readyclosed

I don't want to backport this change - it is only used by developers.

I did want to do backports using the scripts in this change, but I'll do that when I have time, and fix any bugs.

Note: See TracTickets for help on using tickets.