List:Commits« Previous MessageNext Message »
From:Timothy Smith Date:February 15 2008 5:05pm
Subject:Re: bk commit into 5.0 tree (tsmith:1.2578) BUG#28555
View as plain text  
Joerg,

Thanks a lot for the quick review!

On Fri, Feb 15, 2008 at 11:40:20AM +0100, Jörg Brühe wrote:
> I tested the RPM commands which you added on a SuSE 9.3 installation, and 
> they worked fine.

Good!

> I did not try to verify your complete patch, because I lack a test machine 
> (physical or virtual) on which to install a RPM that might trigger the 
> check.

OK.  I'll try to install an older SuSE mysql RPM in a VM.

> tim@stripped wrote:
>> +%define mysql_vendor    MySQL AB
>
> While that is correct, it still leaves the risk of somebody outside MySQL 
> just using the spec file without changing this setting.
> If we really want to be safe, we might set it to "unknown" here and then 
> override it when we start the RPM build.
> This would require additional tool changes (build scripts).

I think leaving it as 'MySQL AB' is the correct approach.  Even if
someone uses our spec file directly, we are still the provider, and the
resulting package will still interact correctly with previous RPMs from
us.  Unless she makes drastic changes to the spec, in which case the
onus is on her to cope with upgrade issues.

>> +  vendor=`rpm -q --queryformat='%{VENDOR}' "$installed" 2>&1`
>> +  version=`rpm -q --queryformat='%{VERSION}' "$installed" 2>&1`
>
> The above commands work fine on SuSE 9.3 with MySQL 5.1.23-rc installed.

Thank you!

> So my proposal is:
>   +
>   +  old_family=`echo $version | sed -n -e 
> 's,^\([1-9][0-9]*\.[0-9][0-9]*\)\..*$,\1,p'`
>   +  new_family=`echo $myversion | sed -n -e 
> 's,^\([1-9][0-9]*\.[0-9][0-9]*\)\..*$,\1,p'`

Agreed.

>> +  if [ -n "$vendor_error$family_error" ]; then
>
> This may be a style issue ...
> I would not combine these two strings into one, rather check them separate, 

You're right, it's not the easiest format for reading the code.

>       if [ -n "$error_string" ]; then
>
> One of my reasons is that this approach will make it easier to extend the 
> checks, should we add more later.

Good point.

>> +    cat <<HERE >&2
>> +
>> +******************************************************************
>> +A MySQL server package ($installed) is installed.
>> +$vendor_error$family_error
>> +A manual upgrade is required.  A recommended procedure is:
>                                  ^^^
> Isn't it "The" recommended procedure ?  Do we have several ?

It's my recommended procedure.  Perhaps I can make it The recommended
procedure by getting the docs team to add it to the manual.  :-)

> I am torn between two positions:
> - Keep it small.
> - Provide as many hints as possible.
>
> For "small", your text is fine.
>
> For "many hints", we would need to give separate explanations for 
> "different vendor" (directory structure may differ, ...) and
> "different family" (run 'mysql_upgrade').

I thought about this as well, and it seemed that no matter how much I
write, there could always be something missing.  I think I can say a bit
more, though, and will try to form a compromise along the lines you
suggested.

Thanks again,

Timothy
-- 
-- Timothy Smith         Production Engineer; Dolores, Colorado, USA
-- MySQL, www.mysql.com     The best DATABASE COMPANY in the GALAXY!

Attachment: [application/pgp-signature]
Thread
bk commit into 5.0 tree (tsmith:1.2578) BUG#28555tim15 Feb
  • Re: bk commit into 5.0 tree (tsmith:1.2578) BUG#28555Joerg Bruehe15 Feb
    • Re: bk commit into 5.0 tree (tsmith:1.2578) BUG#28555Timothy Smith15 Feb
  • Re: bk commit into 5.0 tree (tsmith:1.2578) BUG#28555Sergei Golubchik15 Feb
    • Re: bk commit into 5.0 tree (tsmith:1.2578) BUG#28555Timothy Smith16 Feb