-
Notifications
You must be signed in to change notification settings - Fork 132
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
Add ebuild for the ares emulator #299
base: master
Are you sure you want to change the base?
Conversation
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>
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why patch out the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-lto
as 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.
xdg_desktop_database_update | ||
xdg_icon_cache_update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're part of the project, make sure to add yourself as the maintainer here!
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.