On Thu, Nov 28, 2013 at 1:32 AM, Ben Reser <breser_at_apache.org> wrote:
> 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.
Fair enough. It was just driving me buggy when I was reading it and
updating. This variant is now a couple of years old, and the
consistent layout has made it easier for me to maintain. It makes
posting a "diff" kind of hard to read, but so did the insertion of all
the "return 1" statements for error checking. I've found it easier to
parse as a side by side comparison, not a diff.
>> * 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.
Didn't know about that one, and I suspect we don't want to be reliant
on /bin/bash being available. Drat.
OK, it's split into 2 lines again.
>> * 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.
Fair enough. auto-download is disabled, and I've added a comment
saying "download this specifically if you want it".
While looking at it, I moved the comments for the "mod_dav_svn" and
"apr-iconv" into subroutines, to consistently handle such requests.
>> * 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 was thinking about setting a RETVAL for exit status if the
individual downloads fail. Haven't done it yet.
>> * 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.
Yeah, I have a similar problem with other environments and EPEL
packages. It makes testing a new system with the exact same downloaded
components kind of problematic, but it's understandable that freeware
authors don't want to have to maintain older, buggy releases. It's one
of the reasons I like to actually build RPM's, rather than
auto-compiling things on servers at whim. But when the upstream
repository understandably refuses to keep the previous releases
available, it's awkward to keep testing and deployment environments
stable.
Received on 2013-11-28 14:20:03 CET