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.
