Opened 11 years ago

Closed 11 years ago

Last modified 2 years ago

#4861 closed defect (fixed)

"make" prints warnings if Subversion is not installed

Reported by: fingolfin Owned by: dhewg
Priority: low Component: --Other--
Keywords: Cc:
Game:

Description

Our Makefile.common invokes the "svn" command without checking whether it is even installed. Hence I get this warning when doing "make" without subversion installed:

/bin/sh: svn: command not found

That's bad and ugly; much better to have a test for whether SVN is even installed, and usable with the checkout. For example, I have a ScummVM checkout on a NFS shared home volume, and use it on multiple machines, some with and some without SVN, and some with ancient SVN versions.

Ticket imported from: #2988223. Ticket imported from: bugs/4861.

Attachments (2)

svn_check.patch (1.8 KB ) - added by lordhoto 11 years ago.
Patch against trunk.
svn_check_posix.patch (1.8 KB ) - added by lordhoto 11 years ago.
Patch without the use of "which"

Download all attachments as: .zip

Change History (9)

by lordhoto, 11 years ago

Attachment: svn_check.patch added

Patch against trunk.

comment:1 by lordhoto, 11 years ago

I made a simple patch, which checks for "svn" in configure and sets HAVE_SVN accordingly in config.mk.

In case HAVE_SVN is not set (or non 1), no rules including the "svn" command will be executed anymore. Also the revision is not appended to the CXXFLAGS anymore. I also changed the $(VERFILE) target to throw an error in case svn is not installed, since it relies on "svn export" to be working and I consider it nicer to have some nice error instead of "command not found".

Hopefully someone more into our build system is able to comment on that.

by lordhoto, 11 years ago

Attachment: svn_check_posix.patch added

Patch without the use of "which"

comment:2 by lordhoto, 11 years ago

I just added another patch, which uses "type" (which is POSIX conform) instead of "which" for detecting, whether there is a "svn" in the current PATH.

comment:3 by fingolfin, 11 years ago

Actually, I don't think we need to detect svn using configure. The problem really is this check:

ifeq ($(shell LANG=C svn stat $(srcdir) 2>&1 | grep "is not a working copy"),)

if no command "svn" is installed, it evaluates to *true*, i.e. it acts as if one was in a working copy; and then the second call to svn used for setting VER_SVNREV produces the actual error (easy to see this by changing svn to e.g. svnFOO).

Checking for a working copy using grep this way seems rather hackish anyway (and already required us to add the "LANG=C" to make it work on non-english systems). Instead, we should just check the return value, shouldn't we? For this "svn stat" is not suitable as it seems to always return 0, but the exit code for "svn info" seems to reflect whether one is in a working copy or not.

comment:4 by dhewg, 11 years ago

this should be fixed with r48949.. I hope :)

comment:5 by fingolfin, 11 years ago

Owner: set to dhewg
Resolution: fixed
Status: newclosed

comment:6 by fingolfin, 11 years ago

Works, thanks :)

comment:7 by digitall, 2 years ago

Component: --Other--
Note: See TracTickets for help on using tickets.