Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=583327 --- Comment #10 from Orcan 'oget' Ogetbil <oget.fedora@xxxxxxxxx> 2010-07-21 01:12:17 EDT --- (In reply to comment #8) > Fedora Review clementine 2010-07-20 > Thanks a lot for the review. > - Guidelines say cmake projects should use "make VERBOSE=1": > https://fedoraproject.org/wiki/Packaging/cmake > That makes the build logs, as one can guess, verbose. clementine's logs were verbose by default, but I added it, just in case things change in the future. > - All the source files say "or (at your option) any later version", so it > would make sense to say GPLv3+ and GPLv2+ > I changed it to GPLv3+ and GPLv2+ > - libprojectM-devel is listed twice as BuildRequires (once with a > version and once without) > My sloppiness. Removed. > So you are currently not building the xine engine - but you have > xine-lib-devel as a BuildRequires - is it needed in this case? > Nope, it is a leftover from my experiments. > If you try to enable more engines you get a big warning: > > > The following engines are NOT supported by clementine developers: > > xine qt-phonon > > > > Don't post any bugs if you use them, fix them yourself! > > So I guess you have a reason for not building them for Fedora. > If I remember correctly, they use different engines on different OS'. gst was the default, and it works, so I kept it. :) > Some other BuildRequires seems redundant too - did you remove those > that are only required to build the bundled libraries you do not build > anymore? E.g. glew-devel seems to only be used inside the libprojectM > code. > I think glew-devel is left from the days I was compiling the bundled libprojectM. I think that's the last one though. > This package must have been a nightmare w.r.t. bundled libraries. You > have done an excellent job unbundling the source. Very good job and > congratualations. > Yeah, it was a lot of work, constantly bugging the developers, trying to reason with other upstreams to have the patches accepted, etc. I am sure the review took you some time too. Thanks again for your patience. > > What remains is the universalchardet code. This is tricky. Did you > discuss this with some Fedora packaging people? > > Googling a bit shows that this code is used by quite many projects, > and they all bundle it. It would be interesting to know how many > packages that exist in Fedora and bundle the code. I don't know any > reasonably fast way to figure that out though. > > By doing a repoquery I found one package that installs the > universalchardet headers. In that package (codeblocks-devel) the code > is not compiled into a separate library, but bundled with a lot of > other stuff into a big library that has very many dependencies - so it > is not really a good option to use if you only want to use the > universalchardet. Installing the headerfiles for a bundled library the > way codeblocks-devel does seems wrong anyway. > > In a perfect world, I think this would be best compiled as a shared > library in a separate package, having a common maintained codebase for > all users of the code. But you might have some arguments for treating > this case special. > This library does not have a standalone release by itself. Hence there are no packages for it. People keep copying its code into their projects. I talked to a few people on fedora-devel on IRC. They say xulrunner, gnash etc, everybody is sweeping it under the carpet. We can always switch over if it becomes available as a package. > > ? How do you ensure ownership of /usr/share/icons/hicolor/64x64/apps? > By adding the relevant Requires :) Here is my update: SPEC: http://oget.fedorapeople.org/review/clementine.spec SRPM: http://oget.fedorapeople.org/review/clementine-0.4.2-4.fc13.src.rpm Changelog: 0.4.2-4 - Use: make VERBOSE=1 - License is GPLv3+ and GPLv2+ - Put BRs in alphabetical order - Remove redundant BRs: glew-devel, xine-lib-devel, and the extra libprojectM-devel - Add R: hicolor-icon-theme -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review