On 11/27/13 9:04 PM, Nico Kadel-Garcia wrote:
> * I alphabetized the arguments and the subroutines for consistency and
> legibility:
Let's stick with the functional changes first and worry about the consistency
changes after. Combining them just makes it harder to review the functional
changes.
> * You mentioned that I should not use "$()" syntax. It was helpful to
> get the "printf" for SQLITE_VERSION handling into a single line,
> because printf parsing is a bit odd.. I'm startled if any system
> modern enough to compose Subversion 1.8.5';s dependencies do not
> support that syntax, but but it's easy to unroll that change.
Solaris's /bin/sh doesn't support $() unfortunately. Yes $() is part of POSIX.
We've had users complain about this not working. Unfortunately that means you
have to split that onto 2 lines. I wasn't thrilled about doing that but
couldn't find a portable way to do it in one line.
> * You point out that gtest is not yet used. If nothing uses it, why
> was it ever added to get-deps.sh? I can disable that: I started
> writing my version of this before danielsh pulled out the automatic
> gtest, and hadn't merged back that modification.
We don't always remove work in progress changes that have no impact on end
users. The gtest dependency was added for some work on a C++ binding, which
isn't anywhere close to usable and isn't advertised to anyone.
> * The "return 1" everywhere is to *stop* the individual subroutine,
> but allow it to continue on to the next download. If I see one more
> routine that does a "cd" or "wget"and ignores the result and continues
> merrily on its way, I'm going to get upset. That said, the handling of
> bad command lines is a bit odd, but I didn't elect to try to rewrite
> that.
Agreed about checking results. return without the 1 feels more natural. For
one thing that should preserve the exit value of the last command executed in
the function.
> * I updated the SQLite autoconf tool because the www.sqlite.org
> doesn't list the release in the existing get-deps.sh as still being
> supported. The repository is not browseable, so the only way to find
> that old version is to know that it already exists. That strikes me as
> a recipe for pain, so I grabbed the more resent release.
We need to be really careful about updating sqlite. The version you updated to
may be fine. But I know for instance that you don't want to move to a 3.8.x
version. 3.8.x has some poor performance with our wc databases.
I don't really see the problem with us downloading a file not listed on their
website. Their download links seem to be stable, zlib on the other is a much
bigger problem in this respect because they remove the download link we're
using if the version isn't the current version anymore.
Received on 2013-11-28 07:33:32 CET