Opened 14 months ago

Last modified 5 months ago

#31699 needs_revision defect

Remove unused configure.ac checks

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, 044-can, postfreeze-ok
Cc: Actual Points:
Parent ID: #31698 Points:
Reviewer: catalyst Sponsor:

Description

Here is a little script to find macros in orconfig.h.in that are not actually mentioned in our code:

#/bin/bash
for macroname in $(grep '#undef' orconfig.h.in | awk -e '{ print $2; }') ; do
    git grep -l "$macroname" src >/dev/null || echo "$macroname"
done

Some of these macros are used in system header files, but we can safely remove the autoconf checks for the ones that are not. I think they are:

HAVE_EVENT2_BUFFEREVENT_SSL_H
HAVE_EVENT2_DNS_H
HAVE_EVENT2_EVENT_H
HAVE_EVP_SHA3_256
HAVE_GETPASS
HAVE_HTONLL
HAVE_LIBCAP
HAVE_MALLOC_MALLOC_H
HAVE_MALLOC_NP_H
HAVE_STRUCT_TCP_INFO_TCPI_SND_MSS
HAVE_STRUCT_TCP_INFO_TCPI_UNACKED
HAVE_SYS_SYSLIMITS_H
HAVE_U_CHAR
SRCDIR

Child Tickets

Change History (14)

comment:1 Changed 7 months ago by bduszel

Update results from running nickm's script:

AC_APPLE_UNIVERSAL_BUILD
HAVE_EVENT2_BUFFEREVENT_SSL_H
HAVE_EVENT2_DNS_H
HAVE_EVENT2_EVENT_H
HAVE_EVP_SHA3_256
HAVE_GETPASS
HAVE_HTONLL
HAVE_LIBCAP
HAVE_MALLOC_MALLOC_H
HAVE_MALLOC_NP_H
HAVE_STRUCT_TCP_INFO_TCPI_SND_MSS
HAVE_STRUCT_TCP_INFO_TCPI_UNACKED
HAVE_SYS_SYSLIMITS_H
HAVE_U_CHAR
PACKAGE_URL
SIZEOF_SHA_CTX
SRCDIR
USE_ANDROID
_LARGE_FILES
_MINIX
_POSIX_1_SOURCE
_POSIX_SOURCE
_REENTRANT

Total: 23

I will try to remove autoconf checks for these.

comment:2 in reply to:  1 ; Changed 7 months ago by cypherpunks

Replying to bduszel:

Update results from running nickm's script:

Those are probably the exact same identical results as nickm got originally and nothing changed in the meantime.

The posted list was manually pruned to include only those nickm was confident would be safe to remove, like all the ones starting with HAVE_ are a really safe bet. (in particular, the others like _LARGE_FILES and `_POSIX_SOURCE` and anything starting with an underscore is intended to be used by system headers and might be really important, or might be completely useless, or only have any effect on certain targets).

Some of these macros are used in system header files, but we can safely remove the autoconf checks for the ones that are not. I think they are:

Last edited 7 months ago by cypherpunks (previous) (diff)

comment:3 in reply to:  2 Changed 7 months ago by bduszel

I will focus only on the ones with HAVE_ prefix for now. Thanks cypherpunks.

comment:4 Changed 7 months ago by bduszel

I think I was able to clear some of the entries mentioned by nickm.

There are separate commits for each entry to make it easier to pick/drop changes if needed and also to make the review process easier. I removed checks for:

  • getpass,
  • syslimits,
  • htonll,
  • EVP_sha3_256,
  • malloc,
  • libcap.

Script output after the changes:

AC_APPLE_UNIVERSAL_BUILD
HAVE_EVENT2_BUFFEREVENT_SSL_H
HAVE_EVENT2_DNS_H
HAVE_EVENT2_EVENT_H
HAVE_STRUCT_TCP_INFO_TCPI_SND_MSS
HAVE_STRUCT_TCP_INFO_TCPI_UNACKED
HAVE_U_CHAR
PACKAGE_URL
SIZEOF_SHA_CTX
SRCDIR
USE_ANDROID
_LARGE_FILES
_MINIX
_POSIX_1_SOURCE
_POSIX_SOURCE
_REENTRANT

For now I'm too afraid to remove EVENT2, TCP_INFO and U_CHAR. I need to read about autoconf more to do this. Maybe I will look for some help on #tor-dev and clear the rest.

If someone could take a look at changes made so far that would be great.
MR: https://github.com/torproject/tor/pull/1835

comment:5 Changed 7 months ago by teor

Milestone: Tor: unspecifiedTor: 0.4.4.x-final
Status: newneeds_review
Version: Tor: unspecified

Hi, thanks for this PR!

Someone should review this ticket in the next week or so.

comment:6 Changed 7 months ago by dgoulet

Reviewer: catalyst

comment:7 Changed 7 months ago by catalyst

Started looking at this; still not done reviewing. The libcap check should probably remain because I'm pretty sure we're calling stuff in there, even if we're not conditionalizing based on HAVE_LIBCAP.

comment:8 Changed 7 months ago by catalyst

Status: needs_reviewneeds_revision

Thanks for the patches! It mostly looks good.

The libcap checks should remain. Your removal of the other checks seems safe. Feel free to force-push the branch to remove the commit with the libcap checks.

It should probably have a changes file, because these changes have a risk of causing user-visible changes if someone is attempting to build on a less-common platform.

comment:9 Changed 7 months ago by bduszel

Hi catalyst, thanks for the review.

I pushed the branch without libcap changes. I also added one more commit - creation of "changes" file for this ticket.

MR is already updated, please let me know if I should change anything more.

Thanks!

MR: MR: ​https://github.com/torproject/tor/pull/1835

comment:10 Changed 7 months ago by catalyst

Status: needs_revisionneeds_review

comment:11 in reply to:  9 ; Changed 7 months ago by catalyst

Status: needs_reviewneeds_revision

Replying to bduszel:

Hi catalyst, thanks for the review.

I pushed the branch without libcap changes. I also added one more commit - creation of "changes" file for this ticket.

MR is already updated, please let me know if I should change anything more.

Thanks!

MR: MR: ​https://github.com/torproject/tor/pull/1835

Thanks for the revisions! Mostly looks good.

There are some minor issues with the changes file, found by make check-changes. Please let me know if you want help with fixing it up yourself, or if you'd rather we do it for you. As a starting point, you might want to look at doc/HACKING/CodingStandards.md, scripts/maint/lintChanges.py, and the existing ChangeLog file.

./changes/ticket31699:
	bug number 31699 does not appear
	Unrecognized header: 'Minor cleanup '
Makefile:21877: recipe for target 'check-changes' failed
make[1]: *** [check-changes] Error 1

comment:12 in reply to:  11 Changed 6 months ago by bduszel

Replying to catalyst:

Please let me know if you want help with fixing it up yourself, or if you'd rather we do it for you.

I apologize for the late reply, busy times. Let me try to fix it on my own. I plan to contribute more to the Tor Project so it is important for me to know what's expected and understand the full workflow.

I will let you know as soon as the MR will be updated. Thanks!

comment:13 Changed 5 months ago by nickm

Keywords: 044-can added

Any updates here?

comment:14 Changed 5 months ago by nickm

Keywords: postfreeze-ok added

Mark tickets which are important or safe enough to look at post-freeze for 0.4.4.

Note: See TracTickets for help on using tickets.