Opened 6 years ago

Closed 5 years ago

#8966 closed defect (implemented)

contrib/ directory should be cleaned up

Reported by: rransom Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: 025-triaged, nickm-review-0255
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The contrib/ directory is full of files which are unmaintained and should not be in use. The few files which are known to be required by other parts of the Tor tarball should be moved elsewhere, either to a scripts/ or etc/ directory or to a subdirectory of contrib/ with a meaningful name, and documented.

Packaging files and their documentation belong in other repositories, unless Nick is willing to verify that they are still useful and document and maintain them.

Child Tickets

Attachments (9)

0001-Cleaning-up-the-mess-in-contrib-.-Creating-scripts.patch (362.9 KB) - added by rl1987 6 years ago.
0002-Making-sure-make-check-docs-and-make-check-spaces-wo.patch (5.5 KB) - added by rl1987 6 years ago.
0003-Fixing-typo.patch (737 bytes) - added by rl1987 6 years ago.
clean-up-contrib.sh (3.0 KB) - added by nickm 6 years ago.
clean-up-contrib.sh-commented (4.4 KB) - added by nickm 6 years ago.
clean-up-contrib-v2.sh (3.8 KB) - added by rl1987 6 years ago.
clean-up-contrib-v3.sh (4.2 KB) - added by nickm 5 years ago.
clean-up-contrib-v4.sh (4.0 KB) - added by nickm 5 years ago.
clean-up-contrib-v5.sh (3.7 KB) - added by nickm 5 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-final

comment:2 Changed 6 years ago by rl1987

Status: newneeds_review

Changed 6 years ago by rl1987

Attachment: 0003-Fixing-typo.patch added

comment:3 Changed 6 years ago by nickm

notes to self: use git's -M flag to see what moved and what got deleted.

I need to confirm that build.nsi is really supposed to be gone.

I need to do another pass over this stuff after merging these to see whether there's anything else that should be removed rather than relocated.

I need to 'make distcheck'.

comment:4 Changed 6 years ago by nickm

This needs to be a set of scripts rather than a patch. Here's an automatically reconstructed script to do the the moves and deletes from the above patch series.

Changed 6 years ago by nickm

Attachment: clean-up-contrib.sh added

Changed 6 years ago by nickm

comment:5 Changed 6 years ago by nickm

I've reviewed the new file locations and written some inline comments in the file. We need to investigate "who needs what" for some of these.

comment:6 in reply to:  3 Changed 6 years ago by rl1987

Replying to nickm:

notes to self: use git's -M flag to see what moved and what got deleted.

I need to confirm that build.nsi is really supposed to be gone.

This build.nsi file was added in 2009 (see 0e5973dec27c5c9d4e711423ab1257bb53bdb875) and pretty much hasn't been updated since. Contents of this file indicate it being an NSIS script for early versions of Tor browser bundle. Since we now have a separate subproject for TBB and it's related scripts, build.nsi is not supposed to be included in core Tor codebase.

comment:7 Changed 6 years ago by rl1987

# XXXX Is this used?
git mv contrib/cross.sh contrib/build-tools/cross.sh

This is a cross-compilation helper script that was maintained from 2006 to 2009. It's probably no longer in any use, since there was some effort to have standard autotools-based build system support cross-compilation (see ticket #9869).

# XXXX Do we use this?
git mv contrib/netinst.nsi contrib/build-tools/netinst.nsi
# XXXX Do we use this?
git mv contrib/package_nsis-mingw.sh contrib/build-tools/package_nsis-mingw.sh
# XXXX Do we use this?
git mv contrib/tor.ico contrib/build-tools/tor.ico
# XXXX Do we use this?
git mv contrib/torinst32.ico contrib/build-tools/torinst32.ico
# XXXX Do we use this?
git mv contrib/xenobite.ico contrib/build-tools/xenobite.ico

All of these seem to be related to TBB and Tor expert bundle. Arguably, core Tor codebase is not where they should be, even if they are used (pretty sure they're not, judging from the fact that NSIS stuff was last updated in 2011 and icon files were added in 2006).

# contrib/dir-tools/directory-archive/
# Tools for running a directory archive.  Are they used?
git mv contrib/directory-archive/crontab.sample contrib/dir-tools/directory-archive/crontab.sample
git mv contrib/directory-archive/fetch-all contrib/dir-tools/directory-archive/fetch-all
git mv contrib/directory-archive/fetch-all-v3 contrib/dir-tools/directory-archive/fetch-all-v3
git mv contrib/directory-archive/tar-them-up contrib/dir-tools/directory-archive/tar-them-up
git mv contrib/directory-archive/fetch-all-functions contrib/dir-tools/directory-archive/fetch-all-functions
git mv contrib/directory-archive/sort-into-month-folder contrib/dir-tools/directory-archive/sort-into-month-folder

I don't know. They might be useful to researcher who wants an easy way to download data from Tor directory servers to do some kind of math, but Onionoo web services kinda make these scripts obsolete. Besides, these scripts haven't been updated in a while and may be buggy and/or fail to implement the latest developments in Tor directory protocol.

# XXXX is this used? Does it work?
git mv contrib/mdd.py scripts/mdd.py

This is a Python script to generate call graph for C code. It was added in 2004 and has not been updated since then. It does work if you change the first line to #!/usr/bin/env python, but it is fairly certain nobody uses it anymore. Doxygen being able to generate call-graphs with pretty pictures makes it basically obsolete.

# XXXX is this used? Does it work?
git mv contrib/redox.py scripts/redox.py

This was supposed to solve some sort of problem with Doxygen. I gave it a try and it failed with Python error:

Traceback (most recent call last):
  File "./contrib/redox.py", line 213, in <module>
    comments = checkf(fn, errs)
  File "./contrib/redox.py", line 154, in checkf
    if any(pat.match(name) for pat in SKIP_NAMES):
  File "./contrib/redox.py", line 154, in <genexpr>
    if any(pat.match(name) for pat in SKIP_NAMES):
TypeError: expected string or buffer

comment:8 Changed 6 years ago by nickm

I fixed the redox script a little.

Also, peter and Karsten say they aren't using the directory-archive scripts.

comment:9 Changed 6 years ago by rl1987

# XXX Does anybody use this?
# STET contrib/rc.subr

This seems to be a script for running Tor daemon on FreeBSD system, added in 2006 and last changed in 2009. I believe it safe to remove it.

Changed 6 years ago by rl1987

Attachment: clean-up-contrib-v2.sh added

comment:10 Changed 6 years ago by rl1987

I made changes to script you have generated. I'm still not sure what to do about OS-specific scripts. Should we delete SuSE, Red Hat et. al. related files?

comment:11 Changed 6 years ago by nickm

I'm not so sure about the "it hasn't changed, so remove it" idea. There's no reason that an icon file would change frequently, for example.

As for OS-specific scripts, I think we should actually talk to the maintainers and/or look at their packages to see whether they use those actual scripts or not.

comment:12 Changed 6 years ago by andrea

Keywords: 025-triaged added

comment:13 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

Plan: re-do so that we remove almost nothing now; actually remove stuff in 0.2.6.

comment:14 Changed 5 years ago by nickm

Keywords: nickm-review-0255 added

Changed 5 years ago by nickm

Attachment: clean-up-contrib-v3.sh added

Changed 5 years ago by nickm

Attachment: clean-up-contrib-v4.sh added

comment:15 Changed 5 years ago by nickm

added two more versions of the script. v3 is based on rl1987's v2, for easy diffing. v4 is v3, except sorted by file destination.

I've decided to take the stuff I'm not sure about and move it to contrib/attic. We should have a readme that says it will all get deleted in 0.2.6 unless plans change.

I'd really like to know whether all that .nsi and .ico and cross.sh other stuff is actually used for building any of the vidalia bundles or the expert bundle on windows. But I can't wait another month for that; I think that maybe moving it out of the way will get people to tell me if it should be done.

It's silly that we seem to have 3 or 4 rc.d scripts ; perhaps 1 would be smater. Or 0. But, it doesn't have to be now.

Is the v4 thing "clean enough" ?

Changed 5 years ago by nickm

Attachment: clean-up-contrib-v5.sh added

comment:16 Changed 5 years ago by nickm

Added another version. Damian persuaded me that having an attic is stupid, since we can just restore anything in 0.2.5.6-alpha which we removed. Version control *is* the attic.

Erinn explained that the expert bundles still need package_nsis-mingw.sh, tor.nsi.in, and tor.ico for now, so moving those to win32build.

I expect to merge some version of this tomorrow, then tidy up the debris.

comment:17 Changed 5 years ago by nickm

Added another version. Damian persuaded me that having an attic is stupid, since we can just restore anything in 0.2.5.6-alpha which we removed. Version control *is* the attic.

Erinn explained that the expert bundles still need package_nsis-mingw.sh, tor.nsi.in, and tor.ico for now, so moving those to win32build.

I expect to merge some version of this tomorrow, then tidy up the debris.

comment:18 Changed 5 years ago by nickm

Cleaned up, finished, and merged. Thanks to rl1987 and everybody else who has helped me out here.

comment:19 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_revisionclosed
Note: See TracTickets for help on using tickets.