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

x11-libs/libdlo: New package #7844

Closed
wants to merge 1 commit into from
Closed

Conversation

ConiKost
Copy link
Contributor

@ConiKost ConiKost commented Apr 6, 2018

x11-libs/libdlo: New package

Closes: https://bugs.gentoo.org/652702

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull Request assignment

Areas affected: ebuilds
Packages affected: x11-libs/libdlo

x11-libs/libdlo: @gentoo/proxy-maint (new package)

Bugs linked: 652702

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

@gentoo-repo-qa-bot gentoo-repo-qa-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). bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Apr 6, 2018
@ConiKost
Copy link
Contributor Author

@a17r Would you also have a look here? It's a needed dependency for dev-libs/serdisplib.

Copy link
Member

@a17r a17r left a comment

Choose a reason for hiding this comment

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

Package installs /usr/bin/test1 - that's unfortunately not a very distinctive filename and should be changed to e.g. ${PN}-test (it may make sense to check how other distros do it). If this is just an example or tool it could also be made optional. If this is just for testing the library this should not be installed at all. By the looks of it, the latter seems to be the case.

Unnecessary /usr/lib64/libdlo.la should be removed via the usual find "${D}" -name '*.la' -delete || die in src_install.

Don't install /usr/lib64/libdlo.a unless USE=static-libs was set.

It's still very much under development, but it's possible to write custom applications
to it today (or to libdlo user mode library itself, bypassing udlfb).
And it's also possible to configure X and standard X applications to run on udlfb
but that part is still very distro dependent today. Most people in the DisplayLink
Copy link
Member

Choose a reason for hiding this comment

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

That description seems to be overly long, even if it is called longdescription. At least the last two sentences seem superfluous. And what about the kernel number reference relative to 'to date' - it could be a distraction or make it appear this being a very old (?) package contrary to being 'still very much under development'.

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 changed the description, which shoud now sound better.

@ConiKost
Copy link
Contributor Author

ConiKost commented Apr 23, 2018

Package installs /usr/bin/test1

From what I understand, this tool can be used to test your own displaylink device. I've made an use flag "test-program" for this, on which the installation depends and renamed it to displaylink-test.

Unnecessary /usr/lib64/libdlo.la should be removed via the usual find "${D}" -name '*.la' -delete || die in src_install.

Added to the eBuild.

Don't install /usr/lib64/libdlo.a unless USE=static-libs was set.

Fixed, it depends now on IUSE="static-libs".

Copy link
Member

@a17r a17r left a comment

Choose a reason for hiding this comment

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

probably last nitpick here ;)

# Only build the Displaylink test program, if a user want's it.
# If a user want's it, rename test1 to displaylink-test.
if ! use test-program; then
sed -i -e '/test\/Makefile/d' configure.ac || die
Copy link
Member

Choose a reason for hiding this comment

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

I need to put the obligatory statement here that an (unconditional) patch is usually the preferred method. That said, you can use one sed per file here instead of multiple calls, just use -e for each change.

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 done a patch now :)

# archiver requires 'AM_PROG_AR' in 'configure.ac'.
sed -i -e '/AC_PROG_CC/a AM_PROG_AR' configure.ac || die

# Only build the Displaylink test program, if a user want's it.
Copy link
Member

Choose a reason for hiding this comment

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

no apostrophe in wants, same on next line ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops ;-) Fixed.

sed -i -e '/test\/images.*/d' Makefile.am || die
sed -i -e '/TESTS .*/d' Makefile.am || die
else
sed -i -e 's/TESTS .*/TESTS = test\/displaylink-test/' Makefile.am || die
Copy link
Member

Choose a reason for hiding this comment

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

rather than all that, you could just mv the binary during src_install. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine for me. Just did it this way, as someone told me, that I should avoid using things like mv in src_install and patch the build system.

Copy link
Member

@a17r a17r Apr 23, 2018

Choose a reason for hiding this comment

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

ah, that depends, but this case is trivial. again, a build system patch that you can take upstream is always preferred, so that these workarounds can at some point be dropped. It should not take much convincing upstream that test1 is a bad name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, changed now in newest version.

Closes: https://bugs.gentoo.org/652702
Package-Manager: Portage-2.3.24, Repoman-2.3.6
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2018-04-23 22:19 UTC
Newest commit scanned: 202e1a4
Status: ✅ good

No issues found


Gentoo Mirror & CI services are provided by Michał Górny. The hardware was kindly provided by Todd Goodman. This unofficial service is not associated with Gentoo Infrastructure or Gentoo Foundation.

This service is provided by the service provider "as is" and any express or implied warranties, including, but not limited to, the implied warranties of merchantability and fitness for a particular purpose are disclaimed. In no event shall the service provider be liable for any direct, indirect, incidental, special, exemplary, or consequential damages (including, but not limited to, procurement of substitute goods or services; loss of use, data, or profits; or business interruption) however caused and on any theory of liability, whether in contract, strict liability, or tort (including negligence or otherwise) arising in any way out of the use of this service, even if advised of the possibility of such damage.

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). bug linked Bug/Closes found in footer, and cross-linked with the PR. 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
3 participants