Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

media-tv/tvbrowser-bin: Themeable and easy to use TV Guide (bug #685744) #11996

Closed
wants to merge 1 commit into from

Conversation

hjudt
Copy link
Contributor

@hjudt hjudt commented May 14, 2019

New ebuild media-tv/tvbrowser-bin-4.0.1.ebuild: Themeable and easy to
use TV Guide - written in Java.

Signed-off-by: Harald Judt h.judt@gmx.at

@gentoo-bot
Copy link

Copyright policy change

Please note that on 2018-09-15 Trustees have approved new Gentoo copyright policy. All contributions made to Gentoo need to follow this policy. If you include the Signed-off-by line in your commit message, you indicate that you have read the policy and agree to its terms. For more detailed explanation, please see the new Gentoo copyright policy explained article.

Pull Request assignment

Submitter: @hjudt
Areas affected: ebuilds
Packages affected: media-tv/tvbrowser-bin

media-tv/tvbrowser-bin: @gentoo/proxy-maint (new package)

Linked bugs

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment.


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added new package The PR is adding a new package. self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else) assigned PR successfully assigned to the package maintainer(s). labels May 14, 2019
<!DOCTYPE pkgmetadata SYSTEM "http://www.gentoo.org/dtd/metadata.dtd">
<pkgmetadata>
<maintainer type="project">
<email>java@gentoo.org</email>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you asked if java team wants to maintain this? Add the proxy-maint project otherwise. Nevertheless, person should become before project.

@@ -0,0 +1,45 @@
# Copyright 1999-2019 Gentoo Foundation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This copyright header is old, use repoman to commit so it fixes it for you automatically ;)


EAPI="7"

inherit eutils desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see eutils being used in here, is it?

inherit eutils desktop

DESCRIPTION="Themeable and easy to use TV Guide - written in Java"
HOMEPAGE="http://www.tvbrowser.org"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use https:// since it's available.

KEYWORDS="~amd64 ~x86"

RDEPEND="virtual/jdk:1.8"
DEPEND="${RDEPEND}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doubt java is being used here during emerge, since it just seems to move pre-compiled files.

You can leave this empty or remove it.

src_install()
{
# Copy files and directories
cd "${S}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the current working directory already.

doins -r *

# Generate launcher
local tvbrowser_bin="tvbrowser"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, what you're doing here is unneeded complexity, since it can be done a lot easier.

https://gitweb.gentoo.org/repo/gentoo.git/tree/sys-apps/baselayout-java/baselayout-java-0.1.0-r1.ebuild#n29 This is a nice example how to.
(@gyakovlev)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This honestly does nothing, you can just use plain "tvbrowser" below instead of "${tvbrowser_bin}".

@juippis
Copy link
Member

juippis commented May 14, 2019

Bear in mind lately new package have had the least priority to be checked, so you might be better off trying to commit into GURU overlay meanwhile to work on your ebuild :)

hjudt added a commit to hjudt/gentoo that referenced this pull request May 14, 2019
@hjudt
Copy link
Contributor Author

hjudt commented May 14, 2019

I have corrected the issues you mentioned.

SRC_URI="mirror://sourceforge/project/tvbrowser/TV-Browser%20Releases%20%28Java%208%20and%20higher%29/${PV}/${MY_PN}_${PV}_bin.tar.gz"
LICENSE="GPL-3"
SLOT="0"
KEYWORDS="~amd64 ~x86"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that came to my mind: Since it's a precompiled binary, does it work on amd64 and x86? Have you tested it on both arches? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not tested it on x86, but it is written in java and therefore should be platform-independent. At least it states to be on the homepage. It should probably work on other arches where the java runtime can run.


S="${WORKDIR}"/"${MY_P}"

src_install()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please stick to ebuild.skel format, src_install() {

@juippis
Copy link
Member

juippis commented May 31, 2019

 * Messages for package media-tv/tvbrowser-bin-4.0.1:

 * QA Notice: broken .png files found:
 *    /var/tmp/portage/media-tv/tvbrowser-bin-4.0.1/image/opt/tvbrowser-bin-4.0.1/imgs/Filter16.png: broken IDAT window length

Otherwise emerges beautifully and seems to run really well.

local tvbrowser_bin="tvbrowser"
exeinto /usr/bin
newexe - "${tvbrowser_bin}" <<-_EOF_
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember indentation, and I believe bash is preferred with launchers / scripts like these. (I think sh is a symlink to system default shell interpreter, so go with bash)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed indentation; I thought it would be necessary for the resulting file to have no indentation, but it luckily works without.
As for the png file, I don't think I'd have to do anything about it as a maintainer other than report it upstream.
I've also fixed the shell because we reference /bin/bash later anyway. I've looked it up in other ebuilds and they also use /bin/bash, and since portage depends on /bin/bash too it should be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also removed the tvbrowser_bin local variable.


src_install() {
# Copy files and directories
dodir /opt/"${P}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need dodir if you're using doins. It creates directory automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed this with the latest push.

doins -r *

# Generate launcher
exeinto /usr/bin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executables for stuff from /opt belong in /opt/bin.

Also the typical way of doing that would be:

into /opt
dobin ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but in this case the ebuild is generating the launcher, thus using exeinto and not dobin. However, I've adapted the path to /opt/bin now.

New ebuild media-tv/tvbrowser-bin-4.0.1.ebuild: Themeable and easy to
use TV Guide - written in Java.

Signed-off-by: Harald Judt <h.judt@gmx.at>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2019-07-19 18:02 UTC
Newest commit scanned: 60dcd31
Status: ✅ good

All QA issues have been fixed!

@juippis
Copy link
Member

juippis commented Aug 1, 2019

I cleaned the ebuild a bit. Please take this into upstream

 * QA Notice: broken .png files found:
 *    /tmp/portage/media-tv/tvbrowser-bin-4.0.1/image/opt/tvbrowser-bin-4.0.1/imgs/Filter16.png: broken IDAT window length

and try to get it resolved before next version is released.

@gentoo-bot gentoo-bot closed this in 82b6355 Aug 1, 2019
@hjudt hjudt deleted the tvbrowser-bin branch June 2, 2020 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). new package The PR is adding a new package. self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else)
Projects
None yet
5 participants