FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > ArchLinux > ArchLinux Pacman Development

 
 
LinkBack Thread Tools
 
Old 01-05-2011, 03:54 PM
Dan McGee
 
Default Bring in official color support at last?

2011/1/5 Vojtěch Gondžala <vojtech.gondzala@gmail.com>:
> Dne 5.1.2011 17:01, Sven-Hendrik Haase napsal(a):
>>
>> Hey,
>>
>> I'm not vogo but I know he's been trying to get his work upstream since
>> 3 years and was rejected last time in order to wait for pacman 3.1. We
>> passed that long ago and I think pacman-color should be given another
>> chance to go upstream.
>> The AUR package with the patch is here:
>> http://aur.archlinux.org/packages.php?ID=11827
>>
>> It is a rather popular package so it might make a fine addition.
>>
>> -- Sven-Hendrik
>>
>
> Hi,
>
> I'm not sure that the patch is ideal for upstream, it's need little bit
> refactor. I'm too busy to do it now, but next release is far. I can do some
> changes at the weekend, if other developers want accept this patch.

Oooh, nothing like a hot-button topic to bring traffic to the ML.

Thoughts:
1. Vogo- no rush, it's obviously been baking a while, no need to pull
it out of the oven early so take your time.
2. I think we'd (the core and regular pacman devs) support this if all
done right.
3. We need to ensure this doesn't interfere with i18n concerns,
especially non-latin alphabets. I have no idea about the current
state, but generate the zh_CN.utf8 locale on your box and try running
this patch under that as a quick test.
4. git now does color, in C code, in a pretty platform-agnostic way.
They've even managed to add a emulation layer for Windows. I don't
think we need all of that, but using their code as a resource is
probably a smart idea. See color.h, color.c, and compat/winansi.c in
their codebase.
5. Does this handle redirects to non-terminals OK; is the
configuration simple but effective, etc.
6. Do we have people willing to review large patches like this? Allan,
Nagy, Xavier and I get very bogged down when we are the only people
looking over code changes and lose interest because we don't get to
enjoy writing our own code.
7. Do we have people willing to help Vogo make changes to this patch
once it is submitted and reviewed?
8. A patchset rather than a patch is easier to review, and moving some
of this stuff to a color.c/color.h would be helpful (and move the
color definitions to color.h so you don't need the extern junk).

-Dan
 
Old 01-09-2011, 02:01 PM
Vojtěch Gondžala
 
Default Bring in official color support at last?

Dne 5.1.2011 17:54, Dan McGee napsal(a):

2011/1/5 Vojtěch Gondžala<vojtech.gondzala@gmail.com>:

Dne 5.1.2011 17:01, Sven-Hendrik Haase napsal(a):


Hey,

I'm not vogo but I know he's been trying to get his work upstream since
3 years and was rejected last time in order to wait for pacman 3.1. We
passed that long ago and I think pacman-color should be given another
chance to go upstream.
The AUR package with the patch is here:
http://aur.archlinux.org/packages.php?ID=11827

It is a rather popular package so it might make a fine addition.

-- Sven-Hendrik



Hi,

I'm not sure that the patch is ideal for upstream, it's need little bit
refactor. I'm too busy to do it now, but next release is far. I can do some
changes at the weekend, if other developers want accept this patch.


Oooh, nothing like a hot-button topic to bring traffic to the ML.

Thoughts:
1. Vogo- no rush, it's obviously been baking a while, no need to pull
it out of the oven early so take your time.
2. I think we'd (the core and regular pacman devs) support this if all
done right.
3. We need to ensure this doesn't interfere with i18n concerns,
especially non-latin alphabets. I have no idea about the current
state, but generate the zh_CN.utf8 locale on your box and try running
this patch under that as a quick test.
4. git now does color, in C code, in a pretty platform-agnostic way.
They've even managed to add a emulation layer for Windows. I don't
think we need all of that, but using their code as a resource is
probably a smart idea. See color.h, color.c, and compat/winansi.c in
their codebase.
5. Does this handle redirects to non-terminals OK; is the
configuration simple but effective, etc.
6. Do we have people willing to review large patches like this? Allan,
Nagy, Xavier and I get very bogged down when we are the only people
looking over code changes and lose interest because we don't get to
enjoy writing our own code.
7. Do we have people willing to help Vogo make changes to this patch
once it is submitted and reviewed?
8. A patchset rather than a patch is easier to review, and moving some
of this stuff to a color.c/color.h would be helpful (and move the
color definitions to color.h so you don't need the extern junk).

-Dan



Hi all,

there is my first idea how colorize output of pacman. It's simple and
function. It's using an ansi escape sequencies, I looked for solution in
a GNU coreutils (ls, grep...) and it's same. Diffred solution is used in
utility tput but is needed ncurses and ncurses isn't good dependency for
pacman, in my opinion.


This patch doesn't support configuration of used colors, but it adding
'nocolor' option in pacman.conf and '--nocolor' argument, for disable
colorize output. I tested patch on some locale and works good with all.


Any idea how do it beter?


--
Vojtěch "vogo" Gondžala
 
Old 01-10-2011, 04:47 PM
Dan McGee
 
Default Bring in official color support at last?

2011/1/9 Vojtěch Gondžala <vojtech.gondzala@gmail.com>:
> Dne 5.1.2011 17:54, Dan McGee napsal(a):
>>
>> 2011/1/5 Vojtěch Gondžala<vojtech.gondzala@gmail.com>:
>>>
>>> Dne 5.1.2011 17:01, Sven-Hendrik Haase napsal(a):
>>>>
>>>> Hey,
>>>>
>>>> I'm not vogo but I know he's been trying to get his work upstream since
>>>> 3 years and was rejected last time in order to wait for pacman 3.1. We
>>>> passed that long ago and I think pacman-color should be given another
>>>> chance to go upstream.
>>>> The AUR package with the patch is here:
>>>> http://aur.archlinux.org/packages.php?ID=11827
>>>>
>>>> It is a rather popular package so it might make a fine addition.
>>>>
>>>> -- Sven-Hendrik
>>>>
>>>
>>> Hi,
>>>
>>> I'm not sure that the patch is ideal for upstream, it's need little bit
>>> refactor. I'm too busy to do it now, but next release is far. I can do
>>> some
>>> changes at the weekend, if other developers want accept this patch.
>>
>> Oooh, nothing like a hot-button topic to bring traffic to the ML.
>>
>> Thoughts:
>> 1. Vogo- no rush, it's obviously been baking a while, no need to pull
>> it out of the oven early so take your time.
>> 2. I think we'd (the core and regular pacman devs) support this if all
>> done right.
>> 3. We need to ensure this doesn't interfere with i18n concerns,
>> especially non-latin alphabets. I have no idea about the current
>> state, but generate the zh_CN.utf8 locale on your box and try running
>> this patch under that as a quick test.
>> 4. git now does color, in C code, in a pretty platform-agnostic way.
>> They've even managed to add a emulation layer for Windows. I don't
>> think we need all of that, but using their code as a resource is
>> probably a smart idea. See color.h, color.c, and compat/winansi.c in
>> their codebase.
>> 5. Does this handle redirects to non-terminals OK; is the
>> configuration simple but effective, etc.
>> 6. Do we have people willing to review large patches like this? Allan,
>> Nagy, Xavier and I get very bogged down when we are the only people
>> looking over code changes and lose interest because we don't get to
>> enjoy writing our own code.
>> 7. Do we have people willing to help Vogo make changes to this patch
>> once it is submitted and reviewed?
>> 8. A patchset rather than a patch is easier to review, and moving some
>> of this stuff to a color.c/color.h would be helpful (and move the
>> color definitions to color.h so you don't need the extern junk).
>>
>> -Dan
>
>
> Hi all,
>
> there is my *first idea how colorize output of pacman. It's simple and
> function. It's using an ansi escape sequencies, I looked for solution in a
> GNU coreutils (ls, grep...) and it's same. Diffred solution is used in
> utility tput but is needed ncurses and ncurses isn't good dependency for
> pacman, in my opinion.
>
> This patch doesn't support configuration of used colors, but it adding
> 'nocolor' option in pacman.conf and '--nocolor' argument, for disable
> colorize output. I tested patch on some locale and works good with all.
>
> Any idea how do it beter?

I think Allan might have some things to add, but step 1 is making this
compile with the --enable-debug flag to ./configure:

gcc -std=gnu99 -DLOCALEDIR="/usr/share/locale"
-DCONFFILE="/etc/pacman.conf" -DROOTDIR="/"
-DDBPATH="/var/lib/pacman/" -DCACHEDIR="/var/cache/pacman/pkg/"
-DLOGFILE="/var/log/pacman.log" -DHAVE_CONFIG_H
-DGIT_VERSION="3.4.2-231-gf20d-dirty" -I. -I../..
-I../../lib/libalpm -pedantic -D_GNU_SOURCE -g -O2
-fstack-protector-all -D_FORTIFY_SOURCE=2 -g -Wall -Werror -MT
callback.o -MD -MP -MF .deps/callback.Tpo -c -o callback.o callback.c
cc1: warnings being treated as errors
callback.c: In function ‘cb_log’:
callback.c:664:12: error: ignoring return value of ‘vasprintf’,
declared with attribute warn_unused_result
make[2]: *** [callback.o] Error 1
make[2]: Target `all' not remade because of errors.

Other things:
* Everything I said in my previous email- you need to address these
individually.
* The ":: " removal from a lot of messages- care to explain why? In
some sense, it is probably a good idea to kill it from the strings
themselves, and just prepend it in the yesno code for those so we
actually have it all the time (the cleanup code didn't have it, for
instance). However, this string changes and prepending should
definitely be a separate patch. And you did it on a lot of arbitrary
former printf/INFO messages as well, so breaking this out in a patch
would be good for sure.
* URL, a different color in -Qi/-Si? I think this is overdoing it...
* Why are questions bolded? It doesn't make it stand out any more
since all messages during this time are bold.
* Progress bar coloring? These look totally plain now. I don't want it
to be gaudy, but a light-grey progress meter would be a welcome
addition, /me thinks.
* Do we have to go bold everywhere in addition to color?
* --version & --nocolor do not work together.
* I think you're leaking memory in FREELIST(output); you didn't adjust
the code to also free the contents of the strings in your struct.
* Watch your indents, you have a few spurious changes in here: the "%s
is invalid or corrupted
" is indented improperly, you have a leading
space added to a line in the version(void) code.
* color_vfprintf(): A few thoughts to potentially clean things up:
move COLOR_NONE inside the switch as the same as the default case;
each case should set char color_str == something; have one fprintf()
call after the switch that actually emits the color code; write out
the full "default: break;" for readability.

-Dan
 

Thread Tools




All times are GMT. The time now is 03:10 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org