Bug 1422555

There are a few things we must fix before this package can be approved:

[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/arduino/tools-
     builder/ctags/5.8arduino11, /usr/share/arduino, /usr/share/arduino
     /tools-builder/ctags, /usr/share/arduino/tools-builder

     Does it make sense for this package to own tools-builder? If
     not, should we request that arduino-core provide it?


[!]: Development files must be in a -devel package
     
     You should put the header files into a devel package.


[!]: Package obeys FHS, except libexecdir and /usr/target.

     The shared lib is arch-dependent, and so should go into
     %{_libdir}. Is make install not suffiencent for this package?


One optional suggestion from rpmlint:

arduino-ctags.x86_64: E: incorrect-fsf-address /usr/share/licenses/arduino-ctags/COPYING

You could work with upstream to correct the fsf address in the COPYING file.

Thanks Randy for picking this up!

(In reply to Randy Barlow from comment #1)
> There are a few things we must fix before this package can be approved:
> 
> [!]: Package must own all directories that it creates.
>      Note: Directories without known owners: /usr/share/arduino/tools-
>      builder/ctags/5.8arduino11, /usr/share/arduino, /usr/share/arduino
>      /tools-builder/ctags, /usr/share/arduino/tools-builder
> 
>      Does it make sense for this package to own tools-builder? If
>      not, should we request that arduino-core provide it?

I added the directory ownership here, since I am not sure we want to depend on arduino-core; rather we will require this package from arduino-builder 


> 
> 
> [!]: Development files must be in a -devel package
>      
>      You should put the header files into a devel package.
> 
> 
> [!]: Package obeys FHS, except libexecdir and /usr/target.
> 
>      The shared lib is arch-dependent, and so should go into
>      %{_libdir}. Is make install not suffiencent for this package?

I am not sure which library/headers you are referring to. For the purposes of building arduino sketches I need only the ctags executable, and that has to be placed in the specific directory I am using. This is the reason of the "weird" install step.

> 
> 
> One optional suggestion from rpmlint:
> 
> arduino-ctags.x86_64: E: incorrect-fsf-address
> /usr/share/licenses/arduino-ctags/COPYING
> 
> You could work with upstream to correct the fsf address in the COPYING file.

I will see if upstream can fix it; btw I noticed the ctags rpm package we have in the repos has the same issue.


Spec URL: https://giallu.fedorapeople.org/arduino-ctags.spec
SRPM URL: https://giallu.fedorapeople.org/arduino-ctags-5.8-2.arduino11.fc24.src.rpm

(In reply to Gianluca Sforna from comment #2)
> I am not sure which library/headers you are referring to. For the purposes
> of building arduino sketches I need only the ctags executable, and that has
> to be placed in the specific directory I am using. This is the reason of the
> "weird" install step.

Hello Gianluca!

The library is the .so file:

$ file rpms-unpacked/arduino-ctags-5.8-1.arduino11.fc26.x86_64.rpm/usr/share/arduino/tools-builder/ctags/5.8arduino11/ctags 
rpms-unpacked/arduino-ctags-5.8-1.arduino11.fc26.x86_64.rpm/usr/share/arduino/tools-builder/ctags/5.8arduino11/ctags: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, BuildID[sha1]=0f8f9fa4801c724f6a88c39ad3eb71cdd951b0cc, stripped, with debug_info

That file is an x86_64 file on my machine, so it should not go into /usr/share. Further, it appears to have a main() function - should it go into /usr/bin? If not, it needs to go into %{_libdir}.

The header files are the .h files - typically packages will provide a -devel subpackage that contains those, which is helpful for people who want to link against your package. These files are usually installed under %{_includedir}. For example, python2-devel installs the .h files in /usr/include/python2.7/ on my system.

Sorry - it's not an .so file by extension, but it is a shared object according to file.

If that executable is not meant to be used by humans, it could also go into %{_libexecdir}. It does seem to print out help text when run by itself, which indicates to me that it might be intended for human use. If it's for humans, it should go into %{_bindir}.

Apparently the "file" command returns the same info both for executables and for shared libraries. However, this is indeed an executable, as you can verify by running it with --help option.

Now, this executable is actually a fork of exuberant ctags (also in Fedora) so I could not put it in %{_bindir} or I'd conflict with the ctags RPM. Besides, even if it can be used by humans, the tool generates an index (or tag) file of language objects found in source files that allows these items to be quickly and easily located by a text editor or other utility; so the normal usage is by means of other tools.
However, this particular fork is intended to be used _exclusively_ by Arduino; think of this like a plugin or addon, as reflected by the namespacing (it's arduino-ctags, not just ctags).

We could argue whether it could be a good idea to encourage the Arduino guys to push their changes upstream (I think so) but right now I am just focusing on having a working Arduino update with all the required tools in place.

Lastly, while I think it is possible to move the tool away from the current installation directory, that means additional work to patch the main arduino IDE package in order to let it find the new location. Since no other tool is (and probably never will) using it, I can hardly justify the additional complexity.

I cannot approve this package if it puts arch-dependent files into /usr/share - that's a violation of the FHS and thus a violation of Fedora's packaging guidelines:

https://fedoraproject.org/wiki/Packaging:Guidelines#Filesystem_Layout

I think the best course of action is to move this file to %{_bindir}/arduino-ctags and to patch the IDE to find it in the new location. With this name it does not conflict with the ctags RPM and it's in the correct location.

Ok, done here:

Spec URL: https://giallu.fedorapeople.org/arduino-ctags.spec
SRPM URL: https://giallu.fedorapeople.org/arduino-ctags-5.8-3.arduino11.fc24.src.rpm

Excellent, that looks great. There is still one thing from my first review that hasn't been fixed and I also noticed one more thing upon looking more closely. These must both be fixed to be approved:

[!] Package contains no bundled libraries without FPC exception.

The package contains a subset of glibc in its gnu_regex folder. According to the packaging guidelines[0], you'll need to do a few things by my interpretation:

* Try to get the package to work with Fedora's glibc.
* If the above is not possible for some reason, you must:
  - Put Provides: bundled(glibc) = 2.10.1 into your spec file.
  - Publicly contact upstream to request that they provide a way to use system
    glibc.
  - Document the public outreach in your spec file.


[!]: Development files must be in a -devel package

This is the one I mentioned upon my first review. You need to add an arduino-ctags-devel package that has all the .h files, and installs them into %{_includedir}/arduino-ctags/. However, you should not include the glibc headers from the gnu_regex folder.


You don't have to fix this in order to pass review, but I also recommend it:

[!]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.

I recommend adding comments over your Patch0 and Patch1 lines that give a brief description of the patch, especially the CVE patch. It's a little surprising that the 5.8-11 release from November would not have a CVE from 2014 fixed.


[0] https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries

(In reply to Randy Barlow from comment #9)
> The package contains a subset of glibc in its gnu_regex folder.

Apprently, that folder is used only on non POSIX systems (e.g. Windows) to provide the functions but otherwise are not used. For good measure, I am now removing the directory in the %prep section.


> 
> [!]: Development files must be in a -devel package
> 
> This is the one I mentioned upon my first review. You need to add an
> arduino-ctags-devel package that has all the .h files, and installs them
> into %{_includedir}/arduino-ctags/. However, you should not include the
> glibc headers from the gnu_regex folder.


I thought this was settled in my previous comment. I am not shipping any library here, just a single executable so no library to link against => no -devel needed.


> I recommend adding comments over your Patch0 and Patch1 lines that give a
> brief description of the patch, especially the CVE patch. It's a little
> surprising that the 5.8-11 release from November would not have a CVE from
> 2014 fixed.

Added a couple comments. Both patches are coming from the ctags Fedora package, but I added the CVE one just to avoid bug reports later. As a matter of fact, it affected JavaScript parsing, so it was not really relevant for this package.

Spec URL: https://giallu.fedorapeople.org/arduino-ctags.spec
SRPM URL: https://giallu.fedorapeople.org/arduino-ctags-5.8-4.arduino11.fc24.src.rpm

Created attachment 1263349 [details]
review.txt

Thanks for fixing the gnu_regex bit. I agree that the header files aren't usually  used for executables - I was just thrown off by the wording from fedora-review. I've read over the packaging guidelines and I do not believe the headers are required by them. Thus, the package is approved!

One thing I noticed that I recommend fixing: There's a /usr/lib/.build-id folder being created by the RPM. I recommend removing it after the make install step. Apparently a few other RPMs do this too, so fedora-review sees it as a conflicting owned directory.

Nevermind my comment about the .build-id folder. I learned on the php-devel list that it's apparently being placed there by the rpm build system and not by your spec file:

https://lists.fedoraproject.org/archives/list/php-devel@lists.fedoraproject.org/thread/JET6TZJOAP4XXIBGQ7ICLTKNRYLKGOY7/

Thank you very much Randy. If you ever need a review for one of your packages, feel free to ping me.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/arduino-ctags

arduino-ctags-5.8-4.arduino11.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-2ca713db83

arduino-ctags-5.8-4.arduino11.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-492bf566f8

arduino-ctags-5.8-4.arduino11.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-492bf566f8

arduino-ctags-5.8-4.arduino11.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-2ca713db83

arduino-ctags-5.8-4.arduino11.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

arduino-ctags-5.8-4.arduino11.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.