Bug 724878

Hannes, here are some initial comments:

- Please use the original spelling "TeXstudio" in the %description.

- The tarball bundles a couple of libraries: 
  * hunspell (present in Fedora)
  * qtsingleapplication (present in Fedora)
  * qcodeedit (not present in Fedora)
  As linking bundled libraries is not permitted in Fedora, you have to patch 
  the sources so that the separately packaged libraries are used. If this is
  not possible due to some reason, you might want to ask FPC for a bundled 
  library exception.
  http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

- The license of TeXstudio is GPLv2+, the bundled qtsingleapplication is GPLv3.
  If you get a bundled library exception, the license of the binary would
  be GPLv3.

Alright thanks for your feedback. I will start packaging Qcodeeditor and make note in this bug.
Also I will ask upstream if and how the dependencies could be resolved with libraries already in fedora and without using bundled ones.

Hi,

in parallel with getting qcodeedit reviewed I asked upstream if it is possible to unbundle the libraries. [1]
In case of hunspell and qtsingleapplication there should no big problem but in case of qcodeedit there are some problems since they are using a heavily patched version and it is quite hard to include them upstream.

I try to remove the libraries of hunspell and qtsingleapplication.

With the library of qcodeedit don't really know how to proceed. I see two possible solutions:

1. Ask FPC for an exception for qcodeedit
2. patch qcodeedit with the changes made by the texstudio-devs (don't know if that's possible though)

I'm quite sorry for the probably useless review of qcodeedit.




[1] https://sourceforge.net/projects/texstudio/forums/forum/907839/topic/4622024

Hi Johannes,

(In reply to comment #3)
> With the library of qcodeedit don't really know how to proceed. I see two
> possible solutions:
> 
> 1. Ask FPC for an exception for qcodeedit

Yes, that's the way to go. You should explain in the FPC ticket what (incompatible) changes have been made to qcodeedit and why it's not possible to integrate them in the regular release. 
http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Exceptions


> 2. patch qcodeedit with the changes made by the texstudio-devs (don't know if
> that's possible though)

You should not modify the functionality and/or the API of the original library because this will cause problems to potential future packages that depend on the original sources. Alternatively, the developers of TeXstudio could think of a fork and release their modified qcodeedit separately. But as stated in the  forum post, that obviously isn't an option.


> I'm quite sorry for the probably useless review of qcodeedit.

Not a problem at all. The package might nonetheless be useful to Fedora users/developers.

Alright I asked for help on the -devel list and found someone who helped me out with the patches.

SPEC-URL: http://hannes.fedorapeople.org/texstudio.spec
SRPM-URL: http://hannes.fedorapeople.org/texstudio-2.2-2.fc15.src.rpm

I will file a ticket for the exception as far as I find some time to provide the needed information.

OK, great. To ensure that the bundled libraries are not built and linked, please remove the folders containing the corresponding sources (in %prep).
http://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries#Packages_with_Bundled_Libraries


Some additional minor notes:

- Add a short comment above the License field documenting the multiple
  licensing scenario:
  texstudio binary: GPLv3 due to static linkage of bundled qcodeedit
  texstudio data and image files: GPLv2+

- Instead of requiring texlive-latex, I recommend to use the virtual provides
  tex(latex).

- Drop the following sentence from the %description:
  "You can run it on Windows, Unix/Linux, BSD and MacOSX systems and 
  modify it if you want, since it is licensed under the GPL."

- Prefer removing the dictionary files in %install over excluding them in 
  %files.

Alright filed [1]

SPEC-URL: http://hannes.fedorapeople.org/texstudio.spec
SRPM-URL: http://hannes.fedorapeople.org/texstudio-2.2-3.fc15.src.rpm
Fixed all your notes. Thanks a lot for your helpful comments!


[1] https://fedorahosted.org/fpc/ticket/100

Alright exception was granted today as you can see in the following ticket:
https://fedorahosted.org/fpc/ticket/100

I just added the additional Provides. I don't know if there is something further to take care of. 

SPEC-URL: http://hannes.fedorapeople.org/texstudio.spec
SRPM-URL: http://hannes.fedorapeople.org/texstudio-2.2-4.fc15.src.rpm

Johannes

Here's the formal review of the package. I'm sorry, there are still a few things to fix.

- Add a slash between $RPM_BUILD_ROOT%{_datadir} and %{name} in the rm
  statements. Currently, the files supposed to be removed are still packaged.

- The package provides qm locale files that must be installed with 
  %find_lang %{name} --with-qt and %files -f %{name}.lang. However, as they
  are located in %{_datadir}/texstudio/ instead of a separate subfolder, it's 
  a bit complicated to separate them from the other files in that directory. 
  The qm files must not be added by one of the entries in the %files section.

  Also, texstudio seems to bundle Qt locales (qt_FOO.mq). Please ask upstream
  if they are modified or if they can be removed so that the original locale 
  files that come with Qt are used.

- Please use the original spelling "TeXstudio" in the %description

- Ask upstream to add the GPLv3 license text to the qcodeedit folder. 
  He should also fix the FSF address in COPYING.


$ rpmlint *.rpm
texstudio.src:26: W: unversioned-explicit-provides bundled(qcodeedit)
texstudio.x86_64: E: incorrect-fsf-address /usr/share/doc/texstudio-2.2/COPYING
texstudio.x86_64: E: incorrect-fsf-address /usr/share/texstudio/COPYING
texstudio.x86_64: W: no-manual-page-for-binary texstudio
texstudio-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/texstudio2.2/.ui
texstudio-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/texstudio2.2/.ui
texstudio-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/texstudio2.2/.moc
texstudio-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/texstudio2.2/.moc
3 packages and 0 specfiles checked; 2 errors, 6 warnings.

---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    - GPLv2+ and GPLv3 (bundled and statically linked qcodeedit library)

[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum texstudio-2.2.tar.gz*
    d23cf71c90f3fd950d49bf480285e920  texstudio-2.2.tar.gz
    d23cf71c90f3fd950d49bf480285e920  texstudio-2.2.tar.gz.1

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[.] MUST: If the package does not successfully compile, build or work on an architecture, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[+] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied.
[X] MUST: The spec file MUST handle locales properly.

[+] MUST: If a package installs files below %{_datadir}/icons, the icon cache must be updated.
[.] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
    - exception for qcodeedit has been granted

[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[X] MUST: A Fedora package must not list a file more than once in %files.
    - COPYING and AUTHORS packaged twice due to missing slash in
      rm -rf $RPM_BUILD_ROOT%{_datadir}%{name}/...

[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[.] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
[.] MUST: devel packages must require the base package using a fully versioned dependency.
[+] MUST: Packages must NOT contain any .la libtool archives.
[+] MUST: Packages containing GUI applications must include a %{name}.desktop file. 
[+] MUST: .desktop files must be properly installed with desktop-file-install in the %install section.
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

EPEL <= 5 only:
[+] MUST: The spec file must contain a valid BuildRoot field.
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}.
[+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}.
[.] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'

[X] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
    - upstream should add the GPLv3 license text to the qcodeedit dir

[+] SHOULD: Timestamps of files should be preserved.
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.
[.] SHOULD: Your package should contain man pages for binaries/scripts.

Ok thanks for the review! I asked upstream about the licensing [1] and the qt files [2]. So far I only received a reply for the qt_lang files and they are unmodified, so I removed them.

The missing GPLv3 file is not a blocker or? Do I need to add one?

Also I added the missing file_install macro but I am not sure if it's possible to use %exclude %{_datadir}/texstudio/texstudio_*.qm to exclude them in the file section.


[1] http://sourceforge.net/tracker/?func=detail&aid=3390072&group_id=250595&atid=1126426
[2] http://sourceforge.net/projects/texstudio/forums/forum/907839/topic/4651681

(In reply to comment #10)
> Ok thanks for the review! I asked upstream about the licensing [1] and the qt
> files [2]. So far I only received a reply for the qt_lang files and they are
> unmodified, so I removed them.

OK, sounds good. Please try to build the package and verify whether the Qt locales, e.g. the German ones, are actually used, or if you get untranslated English text snippets somewhere when running the application.

> The missing GPLv3 file is not a blocker or? Do I need to add one?

Nope. It's an upstream thing that should be considered in a future release. There's no further action required on your side.


> Also I added the missing file_install macro but I am not sure if it's possible
> to use %exclude %{_datadir}/texstudio/texstudio_*.qm to exclude them in the
> file section.

Unfortunately, that's not possible. %exclude will also exclude the locale files added by %files -f %{name}.lang. You have to give a list of path expressions that select everything in %{_datadir}/texstudio/ except the .mq files.

Alright I tried to fix everything, please have a look if everything is as expected.

SPEC-URL: http://hannes.fedorapeople.org/texstudio.spec
SRPM-URL: http://hannes.fedorapeople.org/texstudio-2.2-5.fc15.src.rpm

Just found some errors in the .desktop file regarding the path to the icons? Should I ship one which solves this error or should I use desktop-file-install to change the icon path?

Ok fixed it by adding a modified one as additional Source. Mainly it was looking for icons in the wrong directory.
Hope everything is fine now:
SPEC-URL: http://hannes.fedorapeople.org/texstudio.spec
SRPM-URL: http://hannes.fedorapeople.org/texstudio-2.2-6.fc15.src.rpm

OK, the package looks almost fine, but I think we need another iteration... However, it should be the last one.

- Please remove "and %files -f %{name}.lang" from %find_lang. Sorry if my 
  previous comment was misleading. The mentioned addition was supposed to show
  how the %files statement should look.

- I get two messages from desktop-file-validate:

$ desktop-file-validate texstudio.desktop 
texstudio.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated
texstudio.desktop: error: (will be fatal in the future): value "texstudio.png" for key "Icon" in group "Desktop Entry" is an icon name with an extension, but there should be no extension as described in the Icon Theme Specification if the value is not an absolute path

=> drop the Encoding entry and the .png suffix from the .desktop file. The proper file extension is detected automatically.


- Add %dir %{_datadir}/texstudio/ to %files for proper directory ownership.


Some minor improvements:
- Replace "Copying" with "COPYING" in the rm statement, and drop the %exclude 
  from %files.

- Also, add CHANGELOG.txt to the rm statement in %install, remove 
  %{_datadir}/texstudio/CHANGELOG.txt in %files, and add it with %doc.

Ok, think I now also removed the errors I introduced when fixing others during the review ;-)

SPEC-URL: http://hannes.fedorapeople.org/texstudio.spec
SRPM-URL: http://hannes.fedorapeople.org/texstudio-2.2-7.fc15.src.rpm

Thanks for the feedback and help!

OK, I think we're done now. As this is a rename request, I have to explicitly check the presence of a proper Obsoletes/Provides pair, too. So here we go:

- Provides: texmakerx = %{version}-%{release} is present and correct.

- The Obsoletes field is also present and correct, but according to the 
  guidelines, it should look like this: 
  Obsoletes: texmakerx < 2.2-1

http://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages

Please update the Obsoletes field before you check in the package.

----------------
Package APPROVED
----------------

New Package SCM Request
=======================
Package Name: texstudio
Short Description: A feature-rich editor for LaTeX documents
Owners: hannes
Branches: f17 f16 f15
InitialCC:

Git done (by process-git-requests).

Removed redundant f17 branch, f17==devel for approx 6 more months.

Build in rawhide. Thanks for the review!

http://koji.fedoraproject.org/koji/taskinfo?taskID=3270835