Conversation
|
We don’t typically take new packages through pull requests; the easiest way to get this in GURU is to request access and add it yourself (see the contributor guidelines). This also appears to be a fairly involved couple ebuilds, so having you as part of the project to maintain them would be great :) If you’d like some feedback on the ebuilds, I’d suggest dropping into IRC and asking there. I might take a more detailed look later, but we’ve got some much more experienced ebuild writers in IRC. |
|
@demize OK, I've just requested access: https://bugs.gentoo.org/950580 Let's see how this goes. |
demize
left a comment
demize
left a comment
There was a problem hiding this comment.
Went over and left my own thoughts here, but I'm definitely not a cmake expert, so take them with a grain of salt.
librashader support is disabled, because, well... it's Rust. And I don't speak Rust.
I took a look and librashader is... a bit of a mess, especially now that ::gentoo is dropping the nightly USE flag for rust. I could probably get a package built for this, but I don't think I'd be happy with it...
|
|
||
| PATCHES=( | ||
| "${FILESDIR}"/0001-cmake-use-plain-pkg-config-to-discover-zlib-and-libz.patch | ||
| "${FILESDIR}"/0002-cmake-disable-tests.patch |
There was a problem hiding this comment.
The tests build as a binary, that then gets installed. I didn't think that this makes much sense for a "consumer" of the library. Usually there is some flag to disable tests, but nothing here.
Thoughts?
There was a problem hiding this comment.
Makes sense to me! Seems like an unfortunate way to do the tests, but if that’s how they’re doing it, doesn’t make sense to build their test binary here.
It is generally good practice to make sure the tests can run, but that’s probably more effort than it’s worth here, given you’d need to overhaul their build system for them (looks like it doesn’t actually use cmake’s test functionality at all?).
| if ! use gtk; then | ||
| ewarn "The Qt5 GUI of ares is unstable when building ares with LTO." | ||
| ewarn "If you intend to use LTO, then please use the Gtk GUI." | ||
|
|
||
| mycmakeargs+=(-DUSE_QT5=ON) | ||
| fi |
There was a problem hiding this comment.
This seems unnecessary, given you've got LTO disabled in mycmakeargs.
If you want support for LTO, you could add the lto USE flag, and use -DENABLE_IPO=$(usex lto YES NO). (I think just $(usex lto) would work, but I'm not sure if cmake is case-sensitive here.)
There was a problem hiding this comment.
No, that doesn't work. E.g. I have enabled LTO globally in my CFLAGS from make.conf. That alone already breaks the GUI.
What the CMake argument effectively disabled is that ares' build system implicitly messes with the CFLAGS. It doesn't prevent anyone (e.g. me) building the whole thing with LTO enabled. And then wondering why the binary immediately segfaults.
There was a problem hiding this comment.
In general it is customary for ebuilds to:
- unconditionally disable project specific hacks for enabling LTO, if all they do is globally append -flto
- use
filter-ltoas provided by flag-o-matic.eclass if the program displays erroneous behavior such as crashing or otherwise malfunctioning
If the project requires deep knowledge of when to build with lto and when not to (and by that I mean things like building with extremely specific compiler flags, or adding wild and wonky defines, or suppressing certain targets so they are excluded from LTO because they are ancient and decrepit Fortran code that isn't maintained and they intend to port away from it to Cython anyways but haven't gotten around to it yet), you can check using tc-is-lto to determine if the build "should" be using LTO, and then key off of that rather than a USE flag. You probably also then want to use filter-lto after tc-is-lto in order to prevent setting LTO twice (once via a configure option and once via make.conf *FLAGS).
Example: the ffmpeg ebuild.
There was a problem hiding this comment.
@eli-schwartz I did some modifications (inspired by the ffmpeg build) and added some additional warning text. Could you please take another look?
There was a problem hiding this comment.
But why do you need to ewarn at all? Which other packages emit an ewarn for this?
There was a problem hiding this comment.
I don't need the ewarn (as I know how my ebuild works), but I guess it could be interesting for other users? The firefox ebuilds e.g. have a bunch of prints dealing with lto.
Maybe I'm misunderstanding something here, but isn't it always better (in particular on a distro like Gentoo, where the users want to be in control of their system) to tell the user when you're doing something "under the hood", which might change the behaviour of the system?
games-emulation/ares/ares-143.ebuild
Outdated
| xdg_desktop_database_update | ||
| xdg_icon_cache_update |
There was a problem hiding this comment.
You probably want to inherit from xdg, and then you should be able to drop the postrm/postinst functions.
There was a problem hiding this comment.
Since you're part of the project, make sure to add yourself as the maintainer here!
We require this new version for the ares emulator. Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
|
Hmm, given that I didn't get any further feedback: Is this PR now in an acceptable state for merging? |
|
We don't usually merge PRs directly, typically the way GURU contributors use PRs is primarily for feedback. If you've received the feedback you were looking for, feel free to push the changes to the dev branch (along with a It would be a little cleaner if you could squash the changes into a single commit; not necessary, of course. Also, welcome to the project :) |
3 participants
Some love for Super Famicom emulation. ares is more or less the successor of higan, which was the successor of bsnes. We have both bsnes (as a fork) and higan in the upstream Gentoo repo.
I had this ebuild lying around on my system for quite a long time. Lately I tried to bump it, but it didn't work anymore. Turns out the project had migrated from the custom Makefile-based system to a more modern CMake one.
While migrating my ebuild I thought that maybe other people might be interested in it.
I opted to keep the number of useflags low and provide a good config baseline for most people, which resulted in disabling a lot of the audio drivers. Also librashader support is disabled, because, well... it's Rust. And I don't speak Rust.
Another problem is LTO and the Qt5 GUI. It's an upstream problem though, but I'm pointing it out in the ebuild.