Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#2230 closed Bug (fixed)

use the standard version script for the Mac build

Reported by: vasi Owned by: livings124
Priority: Normal Milestone: 1.73
Component: Mac Client Version: 1.72
Severity: Minor Keywords:
Cc:

Description

macosx/version.sh uses 'find' in order to discover SVN_REVISION. It uses the '-E' flag which is only compatible with BSD find. If the user has GNU find first in their $PATH (via Fink or MacPorts?, perhaps), the build will fail.

A simple fix is to use the full path: /usr/bin/find

Attachments (4)

gnufind.patch (609 bytes) - added by vasi 13 years ago.
Patch to use full path
project.patch (1.5 KB) - added by vasi 13 years ago.
Project file diff
version.patch (4.9 KB) - added by vasi 13 years ago.
Unify version.h scripts
version_02.patch (2.5 KB) - added by geirha 13 years ago.
Changes to version.patch

Download all attachments as: .zip

Change History (34)

Changed 13 years ago by vasi

Patch to use full path

comment:1 Changed 13 years ago by charles

  • Component changed from Transmission to Mac Client
  • Owner set to livings124

comment:2 Changed 13 years ago by charles

  • Milestone changed from 1.73 to None Set

Looks like a very simple change, but imo it's up to BMW, since only the mac builds use that script.

comment:3 Changed 13 years ago by livings124

I would still prefer if we use update-version-h.sh like all the other clients, but can't find a way to get it to work for all Mac users from within Xcode. vasi, perhaps you have some insight into this?

comment:4 Changed 13 years ago by vasi

I'd love to help look into this, but if I have Xcode run update-version-h.sh instead of macosx/version.sh, it seems to work fine. (Once I've added VERSION_STRING_INFOPLIST, of course.) There's no problem if I use a non-svn source dir, either. I can't seem to find any tickets about update-version-h.sh failing on OS X. Can you point me to what goes wrong for some people?

comment:5 Changed 13 years ago by livings124

Interesting, I had trouble with this because of the svnrevision call. Could you post a diff?

comment:6 Changed 13 years ago by vasi

Ah, that makes sense! I didn't think about the fact that Mac OS X 10.4 (and lower) doesn't come with svn. How about if we just check whether svn is present?

  • if [ -d .svn ]; then

+ if which svnversion > /dev/null && [ -d .svn ]; then

comment:7 Changed 13 years ago by vasi

Gah, formatting hates me. Here's the diff I'm talking about:

--- update-version-h.sh	(revision 8724)
+++ update-version-h.sh	(working copy)
@@ -5,7 +5,7 @@
 
 peer_id_prefix=`grep m4_define configure.ac | sed "s/[][)(]/,/g" | grep peer_id_prefix  | cut -d , -f 6`
 
-if [ -d .svn ]; then
+if false && [ -d .svn ]; then
     svn_revision=`svnversion -n | cut -d: -f1 | cut -dM -f1 | cut -dS -f1`
 else
     svn_revision=`awk '/\\$Id: /{ if ($4>i) i=$4 } END {print i}' */*.cc */*.[chm]`
@@ -18,4 +18,5 @@
 #define SVN_REVISION_NUM      ${svn_revision}
 #define SHORT_VERSION_STRING  "${user_agent_prefix}"
 #define LONG_VERSION_STRING   "${user_agent_prefix} (${svn_revision})"
+#define VERSION_STRING_INFOPLIST  ${user_agent_prefix}
 EOF

comment:8 Changed 13 years ago by vasi

Er....except it seems recent Transmission builds only on 10.5 anyhow...so now I'm again unsure what's going on.

comment:9 Changed 13 years ago by livings124

We do require 10.5, so don't worry about supporting 10.4. I had problems with the script where I had to change "svnrevision" to a hardcoded path when running using the build button in Xcode. If you have it working in Xcode can you post your diff of the project file?

comment:10 Changed 13 years ago by livings124

vasi: ping

comment:11 Changed 13 years ago by livings124

vasi: ping again

Changed 13 years ago by vasi

Project file diff

comment:12 Changed 13 years ago by vasi

My diff is pretty trivial, I've attached it.

It makes very little sense that you had to hardcode the path to svnversion (I'm assuming that's what you mean by 'svnrevision'?). It can't be missing: it's in /usr/bin, and lots more would be broken if /usr/bin wasn't in the PATH. Moreover, if the user has installed a higher version of svn, and has used that to check out transmission, it's very important that we use the (newer) svnversion that's presumably first in their path--the standard svn 1.4 won't work against newer WC formats.

Any other ideas under what conditions update-version-h.sh may be failing? I'm not sure how useful I can be just guessing about what might be going wrong, without any knowledge of the actual failures that people are seeing.

comment:13 Changed 13 years ago by livings124

Your patch gives me:

update-version-h.sh: line 9: svnversion: command not found

It's the problem I described, where svnversion can't be found when run in Xcode. Are you able to compile with your patch within Xcode GUI?

comment:14 Changed 13 years ago by vasi

Yes, it builds fine for me. I have some additions to my PATH, so I tried removing them, it still works fine.

Could you check your PATH, and make sure svnversion lives in /usr/bin? Also, let's see what PATH is while the script is running in case something weird is going on, make it look like this:

...
if [ -d .svn ]; then
    echo "$PATH" > PATH
    svn_revision=`svnversion -n | cut -d: -f1 | cut -dM -f1 | cut -dS -f1`
else
...

Then you can look at the file 'PATH' in the transmission source dir and see what it says.

comment:15 Changed 13 years ago by livings124

Nope, svnversion is not in /usr/bin (I'm pretty sure the location changed in the latest svn release). And I can guarantee you that if this doesn't work for me, there would be a lot of other users with the same problem.

comment:16 Changed 13 years ago by vasi

Nope, svnversion is not in /usr/bin

I am quite sure that Apple does indeed install /usr/bin/svnversion. I've checked several systems, ranging from PPCs upgraded all the way from OS 9, to a brand new MBP.

Anyhow, if somehow you're missing /usr/bin/svnversion, where is svnversion on your system? I'd really like to see the output of the following commands on your system, to see all the different svnversions, and what the Apple packages are doing:

type -a svnversion
echo "SELECT pkgid, path
    FROM paths JOIN pkgs_paths USING (path_key)
        JOIN pkgs USING (pkg_key)
    WHERE path LIKE '%svnversion';" \
| sudo sqlite3 /Library/Receipts/db/a.receiptdb
pkgutil --file-info /usr/bin/svnversion

Here's what I get on one of my systems:

vasi$ type -a svnversion
svnversion is /sw/bin/svnversion
svnversion is /usr/bin/svnversion
vasi$ echo "SELECT pkgid, path
>     FROM paths JOIN pkgs_paths USING (path_key)
>         JOIN pkgs USING (pkg_key)
>     WHERE path LIKE '%svnversion';" \
> | sudo sqlite3 /Library/Receipts/db/a.receiptdb
com.apple.pkg.BSD|usr/bin/svnversion
vasi$ pkgutil --file-info /usr/bin/svnversion
volume: /
path: usr/bin/svnversion

pkgid: com.apple.pkg.BSD
pkg-version: 10.5.3.1.1.1191932192
install-time: 1222757414
uid: 0
gid: 0
mode: 100755



(I'm pretty sure the location changed in the latest svn release).

'svn release'? Do you mean to say that you installed a non-Apple SVN? That should be fine, I do this too on some systems. But it would be weird for it to have removed the Apple /usr/bin/svnversion. None of Fink, MacPorts? or CollabNet? (to list the ones I've used) do this.



And I can guarantee you that if this doesn't work for me, there would be a lot of other users with the same problem.

I do not dispute this, I'd just like to know under what conditions we can expect this. Does it indicate the the user probably has an svnversion somewhere else? Or that the user may have no SVN at all, somehow? Or what? The proper way of dealing with these scenarios may be different. Thanks for taking the time to help figure this out.

comment:17 Changed 13 years ago by livings124

Yup, I installed a non-Apple SVN (1.6 iirc, but I don't have access to my machine right now). Apparently the new SVN location as of this version is /opt/subversion/bin instead of /usr/local/bin. I manually removed the old svn from /usr/local/bin because that was getting used instead of the new one (perhaps I should have replaced it with an alias, but that's besides the point).

The issue is that svnversion needs a hardcoded path, where when run from Terminal or anywhere else it does not - this is the problem that I would like a better solution to, since relying on svn to be in a specification location isn't really an option.

comment:18 Changed 13 years ago by vasi

Eugh, for future reference it's probably better to rearrange your path, rather than delete Apple binaries. If you just set PATH="/opt/subversion/bin:$PATH" in your shell setup file, rather than PATH="$PATH:/opt/subversion/bin", then the newer subversion will come first. :-)

Anyhow, the reason why you get different results in the Terminal than in Xcode is that the Terminal is a "login shell", and Xcode runs scripts as non-login shells. This means different files get parsed, and if one of those files changes the PATH, it may be ignored.

So we now have a little problem. It's non-trivial to run a command inside a shell that duplicates the environment in the user's terminal. There are many shells the user might prefer (bash, tcsh, zsh...), and their shell may do all kinds of stuff upon login. Maybe it will print a pile of diagnostics, or a fortune. Maybe it will set some quirky shell options.

How about this as a solution? We start with a list of likely places where bin/svnversion may be, let's say: /usr /usr/local /opt /sw /opt/local /opt/subversion. We can try running each of them if they exist, and if any runs are successful we declare success. If none of them work, only then will we try the tricky bit of using a login shell.

Here's a script that should invoke the login shell to find the correct svnversion, regardless of shell. I've tested against sh, bash, tcsh and zsh. As I mentioned above, this isn't trivial, and it's quite possible that there's some situation I've missed where it won't work.

#!/usr/bin/ruby
require 'etc'

shell = Etc.getpwuid(Process.uid).shell
short = File.basename(shell)
# bash goes non-interactive with pipe to STDIN
args = %w[bash sh].include?(short) ? ['-il'] : []
arg0 = '-' + short # let the shell know it's a login shell

outrd, outwr = IO.pipe
inrd, inwr = IO.pipe
children = [
	fork do # writer
		[outrd, outwr, inrd].each { |f| f.close }
		inwr.puts("/usr/bin/which svnversion")
	end,
	fork do # shell
		[outrd, inwr].each { |f| f.close }
		STDIN.reopen(inrd)
		STDOUT.reopen(outwr)
		STDERR.reopen("/dev/null")
		exec([shell, arg0], *args)
	end
]
[outwr, inrd, inwr].each { |f| f.close }
output = outrd.read
children.each { |c| Process.waitpid(c) }

svnversion = output.match(/([^\n]+)\Z/)[1]
puts svnversion

comment:19 Changed 13 years ago by livings124

Perhaps we should just check if subversion exists in any of those common locations, and if it does not, fall back on the header check that the Mac client currently uses, instead of all that craziness. Would it be possible to make a patch of that? (I would really like to standardize how the Mac version and all other versions get SVN info)

And thanks for all of this - it really is helpful!

comment:20 Changed 13 years ago by vasi

We can't do exactly that, but something very similar would work, sure. It's insufficient to check that svnversion exists, because it could be the wrong version of svnversion. We have to check that it both exists, and actually works.

Changed 13 years ago by vasi

Unify version.h scripts

comment:21 Changed 13 years ago by vasi

Ok, I merged the two scripts. The changes to update-version-h.sh:

  • We detect any svnversion that's in the path OR in a set of common dirs
  • If no svnversion is detected, we look for '$Id$' lines using find|awk, from macosx/version.sh. This is slightly changed to look in all dirs; and to use .cc instead of .cpp.
  • The don't-touch-if-unmodified strategy is taken from macosx/version.sh
  • The Stable/Beta/Nightly? detection is taken from macosx/version.sh

I've tested a few scenarious:

  • No svnversion in at all
  • svnversion in PATH
  • svnversion only in common dirs
  • first svnversion found is bad version, later one is ok

This isn't quite perfect yet, there are a few things that need looking at:

  • Are the bash-isms in the script ok? I'm not sure whether all bourne shells support 'type -ap', ditto with ${param:offset:length} syntax.
  • Do we want to do the $Id$ searching with find? It does discover many more files that just '*/*.cc */*.[chm]', but we have to make sure we maintain portability with both BSD and GNU find. Right now it's not Linux compatible, do NOT commit it as it stands.
  • I don't really understand the Stable/Beta/Nightly? thing. Under what situation does the peer_id_prefix become non-'Z'? Is it adjusted manually or something?

comment:22 Changed 13 years ago by livings124

Looks good so far. The script works fine for me.

  1. It's fine for me as long as it works on all operating systems.
  2. Any way to search $Id$ is fine with me, as long as it is reasonably fast and works on all operating systems.
  3. "Beta" is for when we release beta builds, forcing the update feature to check for beta releases regardless of the user setting. The other two are there for consistency. The peer-id prefix is Z for trunk builds, X for beta builds, and 0 for released builds.

Changed 13 years ago by geirha

Changes to version.patch

comment:23 Changed 13 years ago by geirha

I've made some changes to that version.patch.

  • Instead of searching for svnversion, it just checks if svnversion is in path, and falls back to awk if it can't find it.

poking through the system in search for a svnversion that is not in PATH seems overkill to me. If the user has checked out a subversion tree, then the script should assume svn* to be in PATH.

  • substring expansion (${var:x:y}) is indeed bashism, so I changed it to a case instead
  • type -ap is not posix sh either.
  • find will also find the source files under third-party/, which has different revisions than Transmission, I added */*.po to the previous awk instead.

comment:24 Changed 13 years ago by livings124

  • Summary changed from version.sh fails if GNU find is first in path to use the standard version script for the Mac build

comment:25 Changed 13 years ago by vasi

The peer-id prefix is Z for trunk builds, X for beta builds, and 0 for released builds.

Yes, I do understand this part. What I don't get is how this prefix is changed for the different sorts of builds. Is it done manually? Is it part of some sort of build script?

This matters because now we're picking up the peer-id prefix from configure.ac, not from version.sh as before. So for example, if there is some build script that is changing the prefix, it will have to be modified.

I've made some changes to that version.patch.

They look good, thanks.

comment:26 Changed 13 years ago by livings124

Those are entered by hand in configure.ac, at least for beta and release.

comment:27 Changed 13 years ago by livings124

This is working for me. I'm ready to commit it if you guys think it's alright.

comment:28 Changed 13 years ago by livings124

  • Milestone changed from None Set to 1.73

comment:29 Changed 13 years ago by livings124

  • Resolution set to fixed
  • Status changed from new to closed

comment:30 Changed 13 years ago by vasi

Thanks folks, I always appreciate a good response to a bug report :-) . Transmission ftw!

Note: See TracTickets for help on using tickets.