app-misc/lcd4linux: Bump to version 0.11.0_pre20170527 #7846
Conversation
|
Pull Request assignment Areas affected: ebuilds, profiles app-misc/lcd4linux: @gentoo/proxy-maint (maintainer needed) In order to force reassignment and/or bug reference scan, please append |
gentoo-repo-qa-bot
added
maintainer-needed
|
|
||
| LICENSE="GPL-2" | ||
| SLOT="0" | ||
| KEYWORDS="~x86 ~amd64" |
There was a problem hiding this comment.
order keywords alphabetically please.
There was a problem hiding this comment.
Whoops, thanks for the catch. Fixed by newest push.
| # Add supported drivers from 'IUSE_LCD_DEVICES' to 'IUSE' and 'REQUIRED_USE' | ||
| REQUIRED_USE="${REQUIRED_USE} || (" | ||
| for lcd_driver in "${IUSE_LCD_DEVICES[@]}"; do | ||
| IUSE="${IUSE} lcd_devices_${lcd_driver}" |
There was a problem hiding this comment.
You can save a lot of lines and that loop using $(printf 'lcd_devices_%s ' ${IUSE_LCD_DEVICES[@]}) (same for IUSE_LCD4LINUX_PLUGINS below)
There was a problem hiding this comment.
Thanks, changed those blocks now and pushed a new version.
| DEPEND="${RDEPEND}" | ||
|
|
||
| pkg_setup() { | ||
| # If USE="python", call python_setup |
There was a problem hiding this comment.
too trivial a comment to exist, imo.
There was a problem hiding this comment.
I've removed that comment in the newest push.
| src_configure() { | ||
| # Define 'real names from configure' for 'LCD_DEVICES' | ||
| local LCD_DEVICES_NAMES=( ASTUSB BeckmannEgle BWCT CrystalFontz Curses Cwlinux D4D DPF | ||
| EA232graphic EFN FutabaVFD FW8888 G15 GLCD2USB HD44780 HD44780-I2C |
There was a problem hiding this comment.
there seem to be a few indentations too many, imo
There was a problem hiding this comment.
What do you mean exactly?
There was a problem hiding this comment.
Sorry, but I don't see, why there should be too much tabs? I took one tab, but it should start IMHO after "LCD_DEVICES_NAMES". It this wrong?
There was a problem hiding this comment.
We don't use multiple indents just to align with variable names ;) The alignment can never work properly with tabs anyway.
There was a problem hiding this comment.
All right, hope this is okay now :)
|
|
||
| src_install() { | ||
| # Set docs | ||
| local DOCS=( "AUTHORS" "ChangeLog" "CREDITS" "INSTALL" "NEWS" "README" "TODO" ) |
There was a problem hiding this comment.
You can probably remove that line, EAPI6 default should install relevant files by itself.
There was a problem hiding this comment.
Yes, but without my own DOCS, not all of that specified documents are installed.
There was a problem hiding this comment.
Thanks, so I don't changed this.
There was a problem hiding this comment.
The only document that won't be installed is INSTALL... and that makes sense because people don't need to know how to install the package using that instructions (they rely on the ebuild for that). I would then simply drop the line
There was a problem hiding this comment.
Allright ;-) Dropped it and pushed a new version.
| # Set docs | ||
| local DOCS=( "AUTHORS" "ChangeLog" "CREDITS" "INSTALL" "NEWS" "README" "TODO" ) | ||
|
|
||
| # Run 'default' |
| fi | ||
|
|
||
| # Apply user patches | ||
| eapply_user |
There was a problem hiding this comment.
Thanks, applied with newest push.
ConiKost
force-pushed
the
lcd4linux
branch
2 times, most recently
from
280af7b
to
8419837
Compare
| local myconf_lcd_devices | ||
| for lcd_device in ${!IUSE_LCD_DEVICES[@]}; do | ||
| if use lcd_devices_${IUSE_LCD_DEVICES[$lcd_device]}; then | ||
| if [ -z ${myconf_lcd_devices} ]; then |
| local myconf_lcd4linux_plugins | ||
| for lcd4linux_plugin in ${!IUSE_LCD4LINUX_PLUGINS[@]}; do | ||
| if use ${IUSE_LCD4LINUX_PLUGINS[$lcd4linux_plugin]}; then | ||
| if [ -z ${myconf_lcd4linux_plugins} ]; then |
| fi | ||
| done | ||
|
|
||
| # Run 'configure' |
There was a problem hiding this comment.
comment only when something non-obvious is happening ;)
There was a problem hiding this comment.
Well, I like it for defining a new section ;-) Anyway, Fixed.
| # Run 'default', to apply users patches | ||
| default | ||
|
|
||
| # Run 'autotools' |
There was a problem hiding this comment.
comment only when something non-obvious is happening ;)
There was a problem hiding this comment.
Well, I like it for defining a new section ;-) Anyway, Fixed.
| fi | ||
| fi | ||
|
|
||
| # Run 'default', to apply users patches |
There was a problem hiding this comment.
comment only when something non-obvious is happening ;)
There was a problem hiding this comment.
Well, I like it for defining a new section ;-) Anyway, Fixed.
| fi | ||
|
|
||
| # Run 'default', to apply users patches | ||
| default |
There was a problem hiding this comment.
ideally this is run at the very top of src_prepare, before you make other changes. it makes sure upstream patches apply cleanly.
There was a problem hiding this comment.
Moved to top and fixed.
| insopts -m 0600 | ||
| doins lcd4linux.conf.sample | ||
|
|
||
| # Install init script |
There was a problem hiding this comment.
comment only when something non-obvious is happening ;)
There was a problem hiding this comment.
Well, I like it for defining a new section ;-) Anyway, Fixed.
ConiKost
changed the title
| local myconf_lcd_devices | ||
| for lcd_device in ${!IUSE_LCD_DEVICES[@]}; do | ||
| if use lcd_devices_${IUSE_LCD_DEVICES[$lcd_device]}; then | ||
| if [ -z "${myconf_lcd_devices}" ]; then |
There was a problem hiding this comment.
Whoops, I should read correctly ;-) Fixed.
| local myconf_lcd4linux_plugins | ||
| for lcd4linux_plugin in ${!IUSE_LCD4LINUX_PLUGINS[@]}; do | ||
| if use ${IUSE_LCD4LINUX_PLUGINS[$lcd4linux_plugin]}; then | ||
| if [ -z "${myconf_lcd4linux_plugins}" ]; then |
There was a problem hiding this comment.
Whoops, I should read correctly ;-) Fixed.
ConiKost
force-pushed
the
lcd4linux
branch
3 times, most recently
from
2bc3ed7
to
3563258
Compare
| @@ -0,0 +1,327 @@ | |||
| # =========================================================================== | |||
There was a problem hiding this comment.
Please don't put file that huge into FILESDIR.
There was a problem hiding this comment.
Since I will now drop python3, i can drop this one.
|
|
||
| # If no symlink, set instance name to an empty name | ||
| LCD4LINUX_NAME="${SVCNAME}" | ||
| if [ "${LCD4LINUX_NAME}" == "lcd4linux" ]; then |
| return 0 | ||
| } | ||
|
|
||
| start() { |
There was a problem hiding this comment.
Can't you achieve the same using declarational version of openrc scripts? i.e. using variables instead of reinventing start/stop.
There was a problem hiding this comment.
Yep, this works. Rewrote it.
| # The compilation seems also not to link against >=dev-lang/python-3 correctly, so adding it to the libs. | ||
| # See https://lcd4linux.bulix.org/wiki/plugin_python for more information. | ||
| if use python; then | ||
| if ! use python_single_target_python2_7; then |
There was a problem hiding this comment.
Don't rely on those flags explicitly. It's implementation detail, and it's subject to change. Look at EPYTHON instead.
There was a problem hiding this comment.
Okay. Since I will now drop python3, i can drop this one.
| if use python; then | ||
| if ! use python_single_target_python2_7; then | ||
| cp "${FILESDIR}"/ax_python_devel.m4 ax_python_devel.m4 || die | ||
| eapply "${FILESDIR}"/${PN}-0.11.0-add_python3_support.patch |
There was a problem hiding this comment.
Conditional patching is horrible. Can't you submit a proper patch that supports all Python versions upstream?
There was a problem hiding this comment.
Since I will now drop python3, i can drop this one.
| local myconf_lcd_devices | ||
| for lcd_device in ${!IUSE_LCD_DEVICES[@]}; do | ||
| if use lcd_devices_${IUSE_LCD_DEVICES[$lcd_device]}; then | ||
| if [[ -z "${myconf_lcd_devices}" ]]; then |
There was a problem hiding this comment.
There's easier way: just append ,... unconditionally, then strip leading ,.
| if [[ -z "${myconf_lcd_devices}" ]]; then | ||
| myconf_lcd_devices="${LCD_DEVICES_NAMES[$lcd_device]}" | ||
| else | ||
| myconf_lcd_devices="${myconf_lcd_devices},${LCD_DEVICES_NAMES[$lcd_device]}" |
| done | ||
|
|
||
| econf \ | ||
| --disable-rpath \ |
There was a problem hiding this comment.
Please put all of this in an array.
| $(use_with dmalloc) \ | ||
| $(use_with outb) \ | ||
| $(usex python "--with-python PYTHON_VERSION=${EPYTHON#python}" "--without-python") \ | ||
| $(usex lcd_devices_x11 "--with-x" "--without-x") \ |
| $(use_with outb) \ | ||
| $(usex python "--with-python PYTHON_VERSION=${EPYTHON#python}" "--without-python") \ | ||
| $(usex lcd_devices_x11 "--with-x" "--without-x") \ | ||
| $(usex lcd_devices_x11 "--x-include=/usr/include --x-libraries=/usr/$(get_libdir)" "") \ |
There was a problem hiding this comment.
Does it hurt to pass this unconditionally?
ConiKost
force-pushed
the
lcd4linux
branch
4 times, most recently
from
a06227c
to
3e1dfb6
Compare
| # - lcdlinux: It's an ancient driver, which needs app-misc/lcd-linux, that made it never to the portage tree. | ||
| # - lcdlinux: Besides, app-misc/lcd-linux won't compile on a modern linux kernel. | ||
| # - st2205: It's needs dev-libs/libst2205, which made it never to the portage tree and is quite outdated. | ||
| IUSE_LCD_DEVICES=( astusb:ASTUSB beckmannegle:BeckmannEgle bwct:BWCT crystalfontz:CrystalFontz curses:Curses cwlinux:Cwlinux d4d:D4D dpf:DPF |
| KEYWORDS="~amd64 ~x86" | ||
| IUSE="dmalloc outb" | ||
| REQUIRED_USE="lcd_devices_hd44780? ( !lcd_devices_hd44780-i2c ) | ||
| lcd_devices_hd44780-i2c? ( !lcd_devices_hd44780 ) |
There was a problem hiding this comment.
This and the one above it look like a case for ^^ ( ... )
There was a problem hiding this comment.
Thanks, I should have see it by myself :(
| IUSE="dmalloc outb" | ||
| REQUIRED_USE="lcd_devices_hd44780? ( !lcd_devices_hd44780-i2c ) | ||
| lcd_devices_hd44780-i2c? ( !lcd_devices_hd44780 ) | ||
| python? ( ${PYTHON_REQUIRED_USE} )" |
There was a problem hiding this comment.
This is gonna make repoman unhappy.
There was a problem hiding this comment.
Are you sure? Repoman don't complain here about this?
There was a problem hiding this comment.
Sorry, my mistake. I didn't notice you're adding IUSE=python below.
| # Add supported drivers from 'IUSE_LCD_DEVICES' to 'IUSE' and 'REQUIRED_USE' | ||
| REQUIRED_USE+=" || ( " | ||
| for lcd_device in ${IUSE_LCD_DEVICES[@]}; do | ||
| IUSE="${IUSE} lcd_devices_${lcd_device%%:*}" |
There was a problem hiding this comment.
Yeah, found my misstake, why it did not worked last commit. Fixed.
| REQUIRED_USE+=" )" | ||
|
|
||
| # Add supported plugins from 'IUSE_LCD4LINUX_PLUGINS' to 'IUSE' and 'REQUIRED_USE' | ||
| IUSE="${IUSE} $(printf '%s ' ${IUSE_LCD4LINUX_PLUGINS[@]})" |
There was a problem hiding this comment.
Yeah, found my misstake, why it did not worked last commit. Fixed.
| mpris_dbus? ( sys-apps/dbus ) | ||
| mysql? ( || ( dev-db/mariadb-connector-c | ||
| dev-db/mysql-connector-c ) ) | ||
| python? ( ${PYTHON_DEPS} ) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| # Enable all users enabled lcd4linux plugins | ||
| local myeconfargs_lcd4linux_plugins | ||
| for lcd4linux_plugin in "${!IUSE_LCD4LINUX_PLUGINS[@]}"; do |
There was a problem hiding this comment.
You don't have to do the ! indirection here?
There was a problem hiding this comment.
Hm, more, that won't work completly. Rewrote it. I can see now the plugins passed correctly.
| $(use_with outb) | ||
| $(use_with python) | ||
| $(use_with lcd_devices_x11 x) | ||
| --with-drivers="${myeconfargs_lcd_devices:1}" |
There was a problem hiding this comment.
I was actually thinking of ${...#,} as being more 'obvious' but your choice.
There was a problem hiding this comment.
Sounds good. Should be better to read for someone, who is not maintaining the package.
ConiKost
force-pushed
the
lcd4linux
branch
3 times, most recently
from
cedef4a
to
40f2951
Compare
|
Updated |
| # Add supported drivers from 'IUSE_LCD_DEVICES' to 'IUSE' and 'REQUIRED_USE' | ||
| REQUIRED_USE+=" || ( " | ||
| for lcd_device in ${IUSE_LCD_DEVICES[@]}; do | ||
| IUSE+=" lcd_devices_${lcd_device%%:*} " |
There was a problem hiding this comment.
So if you want to play with it further, you can do:
lcd_device=${lcd_device,,}
to make it lowercase. In that case, you wouldn't have to repeat the names if flag == lowercase(upstream-name), i.e. you'd have:
IUSE_LCD_DEVICES=( ASTUSB BeckmannEgle BWCT ... )
There was a problem hiding this comment.
And if you don't want to do that, you can avoid the loop by inverting the order, and doing:
IUSE+=" ${IUSE_LCD_DEVICES[@]/*:/lcd_devices_}"
There was a problem hiding this comment.
Thanks, added the first variant. But the second one IUSE+=" ${IUSE_LCD_DEVICES[@]/*:/lcd_devices_}"seems not to work for me. But I don't even understand it fully.
|
|
||
| # Add supported drivers from 'IUSE_LCD_DEVICES' to 'IUSE' and 'REQUIRED_USE' | ||
| REQUIRED_USE+=" || ( " | ||
| for lcd_device in ${IUSE_LCD_DEVICES[@]}; do |
There was a problem hiding this comment.
Please uppercase global vars, and unset them once no longer needed.
| REQUIRED_USE+=" ) " | ||
|
|
||
| # Add supported plugins from 'IUSE_LCD4LINUX_PLUGINS' to 'IUSE' and 'REQUIRED_USE' | ||
| IUSE+="$(printf '%s ' ${IUSE_LCD4LINUX_PLUGINS[@]})" |
There was a problem hiding this comment.
printf is unnecessary. ${IUSE_LCD4LINUX_PLUGINS[*]} gives you space-sep list.
There was a problem hiding this comment.
Well, printf was here suggested for me ;-) Changed it.
| KEYWORDS="~amd64 ~x86" | ||
| IUSE="dmalloc outb" | ||
| REQUIRED_USE="python? ( ${PYTHON_REQUIRED_USE} ) | ||
| lcd_devices_hd44780? ( !lcd_devices_hd44780-i2c ) |
There was a problem hiding this comment.
I don't get it, how is this supposed to help? ^^ should mean exactly-one-of. But this would mean, I always have to set lcd_devices_hd44780 or lcd_devices_hd44780-i2c?
|
|
||
| # Add supported drivers from 'IUSE_LCD_DEVICES' to 'IUSE' and 'REQUIRED_USE' | ||
| REQUIRED_USE+=" || ( " | ||
| for LCD_DEVICE in ${IUSE_LCD_DEVICES[@]}; do |
There was a problem hiding this comment.
"${IUSE_LCD_DEVICES[@]}". Quote as a good habit when it doesn't make a difference ;-).
ConiKost
force-pushed
the
lcd4linux
branch
2 times, most recently
from
a2a4d59
to
2ed8ca2
Compare
|
Ok, this looks fine and builds fine but requires nmeap. |
I know, there is an open PR #7989 :) |
Closes: https://bugs.gentoo.org/502268 Closes: https://bugs.gentoo.org/652706 Package-Manager: Portage-2.3.40, Repoman-2.3.9
@mgorny: Ok, we should now have all deps in portage. When CI is green, could you have a look for merge? Thanks! |
|
Merged, thanks! Please handle re-keywording now: |
|
app-misc/lcd4linux: Bump to version 0.11.0_pre20170527
Closes: https://bugs.gentoo.org/502268
Closes: https://bugs.gentoo.org/652706
Depends on #7989