You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Nico Kadel-Garcia <nk...@gmail.com> on 2013/11/28 06:04:58 UTC
get-deps.sh patch, was Re: Subversion 1.8.5 released
On Wed, Nov 27, 2013 at 9:13 PM, Ben Reser <br...@apache.org> wrote:
> On 11/27/13 5:09 PM, Nico Kadel-Garcia wrote:
>
>> They're in github as well. I should probably add these comments to the
>> patches, but not tonight.
>>
>> https://raw.github.com/nkadel/subversion-1.8.x-srpm/master/get-deps.sh
>> Updated version of get-deps.sh, with with consistent
>> syntax, latest dependencies, and selection of new sqlite-autoconf
>> package instead of sqlite-amalgamation.
>
> Honestly, I really wish you wouldn't screw with get-deps.sh or would at least
> follow through with my feedback with respect to your suggestion on contributing
> your changes back:
> https://mail-archives.apache.org/mod_mbox/subversion-users/201309.mbox/%3C5231410E.9000701%40reser.org%3E
Ben, I'm not sure I even saw your note way back when. I've retitled
the subject to be about just get-deps.sh, to avoid confusion bout it.
To address the points in your old note about my revision.:
* I alphabetized the arguments and the subroutines for consistency and
legibility:
* 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.
* You mention that apr-iconf is only at version 1.2.1, You're quite
correct, I'm fixing that ASAP.
* 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.
* 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.
* 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.
Re: get-deps.sh patch, was Re: Subversion 1.8.5 released
Posted by Nico Kadel-Garcia <nk...@gmail.com>.
On Thu, Nov 28, 2013 at 6:35 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Ben Reser wrote on Wed, Nov 27, 2013 at 22:32:53 -0800:
>> On 11/27/13 9:04 PM, Nico Kadel-Garcia wrote:
>> > * 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.
>
> Would this not work?
>
> % echo `echo \`echo foo\` | sed s/foo/bar/`
> bar
Brother, the problem is "printf". It doesn't deal with content piped
to it well, and it starts becoming illegible and unmaintenable if you
start playing the "how many backslashes do I need in front of the `"
game. That's why I used "$()", which allows nested subshell
operations..
Oh, well, 2 lines it is.!
Re: get-deps.sh patch, was Re: Subversion 1.8.5 released
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ben Reser wrote on Wed, Nov 27, 2013 at 22:32:53 -0800:
> On 11/27/13 9:04 PM, Nico Kadel-Garcia wrote:
> > * 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.
Would this not work?
% echo `echo \`echo foo\` | sed s/foo/bar/`
bar
Re: get-deps.sh patch, was Re: Subversion 1.8.5 released
Posted by Branko Čibej <br...@wandisco.com>.
On 28.11.2013 07:32, Ben Reser wrote:
> On 11/27/13 9:04 PM, Nico Kadel-Garcia wrote:
>> * 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.
Yes. No-one should just blindly change get-deps.sh to download the
latest and greatest 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.
Not any more, since r1542765. :)
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: get-deps.sh patch, was Re: Subversion 1.8.5 released
Posted by Nico Kadel-Garcia <nk...@gmail.com>.
On Thu, Nov 28, 2013 at 1:32 AM, Ben Reser <br...@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.
Re: get-deps.sh patch, was Re: Subversion 1.8.5 released
Posted by Ben Reser <br...@apache.org>.
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.