Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#19142 closed defect (not a bug)

Remove the environ configure check

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Trivial Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

See ticket:19139#comment:1 on why the environ variable configure check can probably be removed.

Child Tickets

Change History (15)

comment:1 Changed 3 years ago by cypherpunks

It turns out the environ variable cannot be redeclared because of -Wredundant-decls. Instead i looked into why it was added and if it can be removed.

It was initially added in commit efb7b9dec135594718b765234bc1f745855f60d2 because (according to the comment) FreeBSD needed it. The man page (all the way back to FreeBSD 1.0) tells us otherwise. If someone could give the reasoning why FreeBSD requires declaring environ that would be helpful because i don't have a FreeBSD machine and cannot test it myself.

In commit bc66878bdea0250991fc99b2d023146f67a6f4bb the environ configure check was added because it gave redundant declaration warnings on Linux. These were the exact same warnings i got when testing redeclaring environ unconditionally.

comment:2 Changed 3 years ago by nickm

Milestone: Tor: 0.3.0.x-final
Status: newneeds_review

comment:3 Changed 3 years ago by nickm

Keywords: review-group-10 added

Add 0.3.0 needs_review items (and ones that haven't been in needs_revision for very long) to review-group-10.

comment:4 Changed 2 years ago by asn

Status: needs_reviewnew

I don't actually see a patch here to review. Am I missing something? Let's turn it to needs_review when there is one?

comment:5 Changed 2 years ago by cypherpunks

This ticket has been stuck on the following section.

If someone could give the reasoning why FreeBSD requires declaring environ that would be helpful because i don't have a FreeBSD machine and cannot test it myself.

IMO the environ configure can be removed because nobody has given reasons for it being necessary and all the documentation says it is defined by default.

comment:6 Changed 2 years ago by nickm

Keywords: review-group-10 removed

comment:7 Changed 2 years ago by nickm

Status: newneeds_review

ticket19142 does the obvious thing here.

comment:8 Changed 2 years ago by cypherpunks

LGTM

comment:9 Changed 2 years ago by nickm

Status: needs_reviewmerge_ready

comment:10 Changed 2 years ago by nickm

merged!

comment:11 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

comment:12 Changed 2 years ago by rubiate

On OpenBSD:

  CC       src/common/compat.o
src/common/compat.c: In function 'get_environment':
src/common/compat.c:2404: error: 'environ' undeclared (first use in this function)
src/common/compat.c:2404: error: (Each undeclared identifier is reported only once
src/common/compat.c:2404: error: for each function it appears in.)
*** Error 1 in . (Makefile:3699 'src/common/compat.o': @echo "  CC      " src/common/compat.o;depbase=`echo src/common/compat.o | sed 's|[^/...)

comment:13 Changed 2 years ago by nickm

Resolution: implemented
Status: closedreopened

Okay. Reverting this with 2a00110e5bd3592ff69e659681b9294285a98dd0 .

Thanks also for the documentation link as http://man.openbsd.org/?query=environ&apropos=0&sec=0&arch=default&manpath=OpenBSD-current .

comment:14 Changed 2 years ago by nickm

Resolution: not a bug
Status: reopenedclosed

comment:15 Changed 2 years ago by cypherpunks

Sorry for the time wasted on my suggestion which didn't work out in the end.

The FreeBSD builds on the BSD buildbot also started failing on this so i took another look at the FreeBSD implementation. This time i looked at the printenv implementation and it shows on line 55 they also define environ.

However, i believe the environ check can be simplified and I've opened #21023 for that (and other checks).

Note: See TracTickets for help on using tickets.