diff mbox

[1/1] UBUNTU: 'fdr editconfig' modification. Easily skip over unwanted menuconfigs.

Message ID 4C04D6E3.3070205@canonical.com
State Accepted
Delegated to: Leann Ogasawara
Headers show

Commit Message

Lee Jones June 1, 2010, 9:46 a.m. UTC 
>
> > A few notes:
> >
> > I don't think we would generally consider changes like this to a
> > released kernel, so this should probably be applied to maverick. If we
> > do want to put it in lucid, it makes sense to first put it in maverick
> > anyways. As a side note, because we maintain many branches of releases,
> > it helps to prepend the email subject line with "[MAVERICK]" so everyone
> > knows what the patch is for.
> >   
>   
I have seen the "[LUCID]" and "[MAVERICK]" tags and did contemplate
using them, but I had no idea where this patch should go, so
intentionally omitted them.


> > There's a lot of whitespace changes here. If you meant to introduce
> > formatting changes, it's best to do so as a separate patch. 
>   
Noted.


> > Now I can better tell what's going on :). This seems good to me. The
> > previous behavior is maintained (press enter to edit the config file),
> > while giving the user the option to skip editing if they know nothing
> > should change. 
>   
Thank you. That is the behavior I intended.


> > If you can resend a patch like this with only the salient
> > changes I will ACK it. 
>   
How's this:

The following changes since commit f0819aaf4948e34a44d9d685615ddee74271cd70:
  Chase Douglas (1):
        UBUNTU: enforce CONFIG_TMPFS_POSIX_ACL=y

are available in the git repository at:

  git://kernel.ubuntu.com/lag/ubuntu-lucid.git editconfig

Lee Jones (1):
      UBUNTU: 'fdr editconfig' modification. Easily skip over unwanted menuconfigs.

 debian/scripts/misc/kernelconfig |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)




> > hen you do, please check that your indentation
> > style matches the style used throughout the rules files. I'm not sure
> > the above is conforming.
> >   
>   
This is an odd one, as the original file did not conform, even to itself.

For instance; the two case statements:

One indents and uses quotes around the variable:
--------------------------------------------------------------
<snip>
    case "$arch" in
        amd64)    kernarch="x86_64"    ;;
</snip>
--------------------------------------------------------------

The other does neither:
--------------------------------------------------------------
<snip>
    case $config in
    *)
</snip>
--------------------------------------------------------------

The remainder of the kernel doesn't indent, but does use quotes and
Emacs tells me it should be indented.

Your call?

Kind regards,
Lee

Comments

Lee Jones June 1, 2010, 9:55 a.m. UTC | #1 
> This is an odd one, as the original file did not conform, even to itself.
>
> For instance; the two case statements:
>
> One indents and uses quotes around the variable:
> --------------------------------------------------------------
> <snip>
>     case "$arch" in
>         amd64)    kernarch="x86_64"    ;;
> </snip>
> --------------------------------------------------------------
>
> The other does neither:
> --------------------------------------------------------------
> <snip>
>     case $config in
>     *)
> </snip>
> --------------------------------------------------------------
>
> The remainder of the kernel doesn't indent, but does use quotes and
> Emacs tells me it should be indented.
>
> Your call?
>
>   

Oh, and the BASH manual 'does' indent.

BASH & Emacs say "do indent".
Linux kernel says "don't indent".

I'll not submit a format change request until I have a definitive answer
from you guys.

Kind regards,
Lee
Chase Douglas June 1, 2010, 12:28 p.m. UTC | #2 
On Tue, 2010-06-01 at 10:46 +0100, Lee Jones wrote:
> I have seen the "[LUCID]" and "[MAVERICK]" tags and did contemplate
> using them, but I had no idea where this patch should go, so
> intentionally omitted them.

Very little goes directly into an already released kernel. Usually, bug
fixes are sent to stable@kernel.org, then trickle back down to us. If
the change fixes a really important issue, then we may take it in before
that, but this is the exception rather than the rule.

For development kernels, things are usually flipped. We normally apply
changes as we submit them to the stable queue upstream.

In this instance, where you're resubmitting a patch, I would send it as
a new email (not a reply) with something like the following for a
subject:

[MAVERIC][PATCH v3] UBUNTU: 'fdr editconfig' modification. Easily skip
over unwanted menuconfigs.

Generally, the patch numbering is omitted in a single patch instance.

> How's this:
> 
> The following changes since commit f0819aaf4948e34a44d9d685615ddee74271cd70:
>   Chase Douglas (1):
>         UBUNTU: enforce CONFIG_TMPFS_POSIX_ACL=y
> 
> are available in the git repository at:
> 
>   git://kernel.ubuntu.com/lag/ubuntu-lucid.git editconfig

This needs to be applied to the maverick source tree instead.

> 
> Lee Jones (1):
>       UBUNTU: 'fdr editconfig' modification. Easily skip over unwanted menuconfigs.
> 
>  debian/scripts/misc/kernelconfig |   21 +++++++++++++++------
>  1 files changed, 15 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/debian/scripts/misc/kernelconfig b/debian/scripts/misc/kernelconfig
> index 71c0f5e..3181978 100755 (executable)
> --- a/debian/scripts/misc/kernelconfig
> +++ b/debian/scripts/misc/kernelconfig
> @@ -51,9 +51,6 @@ for arch in $archs; do
>                 *)      kernarch="$arch"        ;;
>         esac
>  
> -       echo ""
> -       echo "***************************************"
> -       echo "* Processing $arch ($kernarch) ... "
>         archconfdir=$confdir/$arch
>         flavourconfigs=$(cd $archconfdir && ls config.flavour.*)
>  
> @@ -96,9 +93,21 @@ for arch in $archs; do
>                                 make O=`pwd`/build ARCH=$kernarch oldconfig ;;
>                             editconfig)
>                                 # Interactively edit config parameters
> -                               echo " * Run menuconfig on $arch/$config... Press a key."
> -                               read
> -                               make O=`pwd`/build ARCH=$kernarch menuconfig ;;
> +                               while : ; do
> +                                       echo -n "Do you want to edit config: $arch/$config? [Y/n] "
> +                                       read choice
> +                                       
> +                                       case "$choice" in
> +                                       y* | Y* | "" )
> +                                               make O=`pwd`/build ARCH=$kernarch menuconfig
> +                                               break ;;
> +                                       n* | N* )
> +                                               break ;;
> +                                       *)
> +                                               echo "Entry not valid"
> +                                       esac
> +                               done
> +                               ;;
>                             *)  # Bad!
>                                 exit 1 ;;
>                         esac
> 
> > > hen you do, please check that your indentation
> > > style matches the style used throughout the rules files. I'm not sure
> > > the above is conforming.
> > >   
> >   
> This is an odd one, as the original file did not conform, even to itself.
> 
> For instance; the two case statements:
> 
> One indents and uses quotes around the variable:
> --------------------------------------------------------------
> <snip>
>     case "$arch" in
>         amd64)    kernarch="x86_64"    ;;
> </snip>
> --------------------------------------------------------------
> 
> The other does neither:
> --------------------------------------------------------------
> <snip>
>     case $config in
>     *)
> </snip>
> --------------------------------------------------------------
> 
> The remainder of the kernel doesn't indent, but does use quotes and
> Emacs tells me it should be indented.

It's no fun when things aren't standardized :). I would just try to
reproduce the same style as the code around your changes. In this
instance, the only thing that stands out to me is the case statement
should be like:

case "$choice" in
    y* | Y* | "" )
        make O=`pwd`/build ARCH=$kernarch menuconfig
        break ;;

This wouldn't be my first choice, but it's better to keep things
consistent so it's readable. Other than that, I think everything is
good.

-- Chase
Stefan Bader June 1, 2010, 1:26 p.m. UTC | #3 
On 06/01/2010 02:28 PM, Chase Douglas wrote:
> On Tue, 2010-06-01 at 10:46 +0100, Lee Jones wrote:
>> I have seen the "[LUCID]" and "[MAVERICK]" tags and did contemplate
>> using them, but I had no idea where this patch should go, so
>> intentionally omitted them.
> 
> Very little goes directly into an already released kernel. Usually, bug
> fixes are sent to stable@kernel.org, then trickle back down to us. If
> the change fixes a really important issue, then we may take it in before
> that, but this is the exception rather than the rule.
>

Not completely true. Changes to our build environment might be taken for
convenience as they have no impact after things are build and that can be well
verified. This one even have no impact on the build itself.
But preferably this gets first into Maverick and then there is another
submission if it is deemed useful for any previous release.
In this special case there could be an "Ignore: yes" to hide it from the
generated changelog, given it is something an end-user does not care much about.
This would also prevent the need of a launchpad bug associated, which is
required for SRU otherwise.


> For development kernels, things are usually flipped. We normally apply
> changes as we submit them to the stable queue upstream.

Not that Greg would be too happy to see a change to debian/rules which he could
not care less. ;-P

> In this instance, where you're resubmitting a patch, I would send it as
> a new email (not a reply) with something like the following for a
> subject:
> 
> [MAVERIC][PATCH v3] UBUNTU: 'fdr editconfig' modification. Easily skip
> over unwanted menuconfigs.

Right, that helps to keep the overview what is current or not. Otherwise there
is a long thread with patches attached and one gets confused.

-Stefan

> Generally, the patch numbering is omitted in a single patch instance.
> 
>> How's this:
>>
>> The following changes since commit f0819aaf4948e34a44d9d685615ddee74271cd70:
>>   Chase Douglas (1):
>>         UBUNTU: enforce CONFIG_TMPFS_POSIX_ACL=y
>>
>> are available in the git repository at:
>>
>>   git://kernel.ubuntu.com/lag/ubuntu-lucid.git editconfig
> 
> This needs to be applied to the maverick source tree instead.
> 
>>
>> Lee Jones (1):
>>       UBUNTU: 'fdr editconfig' modification. Easily skip over unwanted menuconfigs.
>>
>>  debian/scripts/misc/kernelconfig |   21 +++++++++++++++------
>>  1 files changed, 15 insertions(+), 6 deletions(-)
>>
>>
>> diff --git a/debian/scripts/misc/kernelconfig b/debian/scripts/misc/kernelconfig
>> index 71c0f5e..3181978 100755 (executable)
>> --- a/debian/scripts/misc/kernelconfig
>> +++ b/debian/scripts/misc/kernelconfig
>> @@ -51,9 +51,6 @@ for arch in $archs; do
>>                 *)      kernarch="$arch"        ;;
>>         esac
>>  
>> -       echo ""
>> -       echo "***************************************"
>> -       echo "* Processing $arch ($kernarch) ... "
>>         archconfdir=$confdir/$arch
>>         flavourconfigs=$(cd $archconfdir && ls config.flavour.*)
>>  
>> @@ -96,9 +93,21 @@ for arch in $archs; do
>>                                 make O=`pwd`/build ARCH=$kernarch oldconfig ;;
>>                             editconfig)
>>                                 # Interactively edit config parameters
>> -                               echo " * Run menuconfig on $arch/$config... Press a key."
>> -                               read
>> -                               make O=`pwd`/build ARCH=$kernarch menuconfig ;;
>> +                               while : ; do
>> +                                       echo -n "Do you want to edit config: $arch/$config? [Y/n] "
>> +                                       read choice
>> +                                       
>> +                                       case "$choice" in
>> +                                       y* | Y* | "" )
>> +                                               make O=`pwd`/build ARCH=$kernarch menuconfig
>> +                                               break ;;
>> +                                       n* | N* )
>> +                                               break ;;
>> +                                       *)
>> +                                               echo "Entry not valid"
>> +                                       esac
>> +                               done
>> +                               ;;
>>                             *)  # Bad!
>>                                 exit 1 ;;
>>                         esac
>>
>>>> hen you do, please check that your indentation
>>>> style matches the style used throughout the rules files. I'm not sure
>>>> the above is conforming.
>>>>   
>>>   
>> This is an odd one, as the original file did not conform, even to itself.
>>
>> For instance; the two case statements:
>>
>> One indents and uses quotes around the variable:
>> --------------------------------------------------------------
>> <snip>
>>     case "$arch" in
>>         amd64)    kernarch="x86_64"    ;;
>> </snip>
>> --------------------------------------------------------------
>>
>> The other does neither:
>> --------------------------------------------------------------
>> <snip>
>>     case $config in
>>     *)
>> </snip>
>> --------------------------------------------------------------
>>
>> The remainder of the kernel doesn't indent, but does use quotes and
>> Emacs tells me it should be indented.
> 
> It's no fun when things aren't standardized :). I would just try to
> reproduce the same style as the code around your changes. In this
> instance, the only thing that stands out to me is the case statement
> should be like:
> 
> case "$choice" in
>     y* | Y* | "" )
>         make O=`pwd`/build ARCH=$kernarch menuconfig
>         break ;;
> 
> This wouldn't be my first choice, but it's better to keep things
> consistent so it's readable. Other than that, I think everything is
> good.
> 
> -- Chase
> 
>
diff mbox

Patch

diff --git a/debian/scripts/misc/kernelconfig b/debian/scripts/misc/kernelconfig
index 71c0f5e..3181978 100755 (executable)
--- a/debian/scripts/misc/kernelconfig
+++ b/debian/scripts/misc/kernelconfig
@@ -51,9 +51,6 @@  for arch in $archs; do
                *)      kernarch="$arch"        ;;
        esac
 
-       echo ""
-       echo "***************************************"
-       echo "* Processing $arch ($kernarch) ... "
        archconfdir=$confdir/$arch
        flavourconfigs=$(cd $archconfdir && ls config.flavour.*)
 
@@ -96,9 +93,21 @@  for arch in $archs; do
                                make O=`pwd`/build ARCH=$kernarch oldconfig ;;
                            editconfig)
                                # Interactively edit config parameters
-                               echo " * Run menuconfig on $arch/$config... Press a key."
-                               read
-                               make O=`pwd`/build ARCH=$kernarch menuconfig ;;
+                               while : ; do
+                                       echo -n "Do you want to edit config: $arch/$config? [Y/n] "
+                                       read choice
+                                       
+                                       case "$choice" in
+                                       y* | Y* | "" )
+                                               make O=`pwd`/build ARCH=$kernarch menuconfig
+                                               break ;;
+                                       n* | N* )
+                                               break ;;
+                                       *)
+                                               echo "Entry not valid"
+                                       esac
+                               done
+                               ;;
                            *)  # Bad!
                                exit 1 ;;
                        esac