View Issue Details

IDProjectCategoryView StatusLast Update
0000059Cinelerra-GG[All Projects] Bugpublic2019-01-02 18:54
Reporterferdnyc Assigned Togoodguy  
PrioritynormalSeveritytweakReproducibilityalways
Status closedResolutionreopened 
Product Version2018-11 
Target VersionFixed in Version2018-12 
Summary0000059: Packaging: Issues identified in source tree / build process
DescriptionThe review process for Cinelerra-GG's inclusion in RPMfusion continues apace (see package review bug):
https://bugzilla.rpmfusion.org/show_bug.cgi?id=5093

Our always-diligent package reviewers have identified a number of issues, most relatively minor, that we've had to work around in packaging the software to meet Fedora guidelines. Policy is that we also bring these issues to the attention of the upstream project, so here I am.

(1) The text of the GPL in cinelerra-5.1/COPYING is out of date, and uses an out of date street address for the FSF offices.

(2) The application's .desktop file template at cinelerra-5.1/image/cin.desktop is outdated, and does not follow the current Freedesktop specification:
http://freedesktop.org/wiki/Specifications/desktop-entry-spec

The following errors are produced by the desktop-file-validate tool:

$ desktop-file-validate image/cin.desktop
image/cin.desktop: warning: value "Application;AudioVideo;Multimedia;VideoEditing;" for key "Categories" in group "Desktop Entry" contains a deprecated value "Application"

image/cin.desktop: error: value "Application;AudioVideo;Multimedia;VideoEditing;" for key "Categories" in group "Desktop Entry" contains an unregistered value "Multimedia"; values extending the format should start with "X-"

image/cin.desktop: error: value "Application;AudioVideo;Multimedia;VideoEditing;" for key "Categories" in group "Desktop Entry" contains an unregistered value "VideoEditing"; values extending the format should start with "X-"

image/cin.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated

image/cin.desktop: error: (will be fatal in the future): value "cin.svg" 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

(3) Many installed files have their executable bits set unnecessarily.

(4) Certain files are built with executable stack, a dangerous code practice which is normally not necessary and merely the result of an oversight in development.

Thanks very much, as I said I'll report 0000003 separately (I have patches to fix it), and I think that should be about it for now.
Additional Information(1) If you wish to continue with the GPLv2, a more current version is found here:
https://www.gnu.org/licenses/old-licenses/gpl-2.0.txt

Or you can switch to GPLv3, which wisely eschews the physical address in favor of a URL, since it's the 21st Century and all:
https://www.gnu.org/licenses/gpl-3.0.txt


(2) Currently I'm fixing up the .desktop file during packaging with the following desktop-file-edit command:

$ desktop-file-edit --set-icon="cinelerra-gg" --remove-key=Encoding \
    --remove-category=Application --remove-category=VideoEditing \
    --remove-category=Multimedia --add-category=AudioVideoEditing \
    --add-category=X-Multimedia $RPM_BUILD_ROOT%{_datadir}/applications/%{name}.desktop

I'll attach a patch which makes the same modifications to the file in the source tree, which would make this step unnecessary. (The patch changes the Icon field from "Icon=cin.svg" to "Icon=cin", since at that stage the file is only a template. The important part of that change is that icon names should be listed in the .desktop file without any extension, since the exact file format used is left as an implementation decision.)

After these modifications, the file passes the desktop-file-validate checks.

(Note: While I've migrated the "Multimedia" category listed in the original .desktop file to "X-Multimedia", both here and in the attached patch, it's not a standard category defined in the Freedesktop standard. As such, it probably isn't going to be recognized anywhere, and simply removing it may be the better course of action.

(3) The install process uses the cinelerra-5.1/inst.sh script to recursively install directory trees with the executable bits set on every file, resulting in most of /usr/lib64/cinelerra-gg/ and large chunks of /usr/share/cinelerra-gg/ containing unnecessarily executable files, even things like the image files in /usr/share/cinelerra-gg/models/. From a packaging standpoint this is not acceptable. I'll file a separate report to address this issue in particular.

(4) Our automated execstack checks report the following:


cinelerra-gg.x86_64: W: executable-stack /usr/bin/cinelerra-gg
cinelerra-gg.x86_64: W: executable-stack /usr/lib64/cinelerra-gg/plugins/themes/theme_blond.plugin
cinelerra-gg.x86_64: W: executable-stack /usr/lib64/cinelerra-gg/plugins/themes/theme_blond_cv.plugin
cinelerra-gg.x86_64: W: executable-stack /usr/lib64/cinelerra-gg/plugins/themes/theme_blue.plugin
cinelerra-gg.x86_64: W: executable-stack /usr/lib64/cinelerra-gg/plugins/themes/theme_blue_dot.plugin
cinelerra-gg.x86_64: W: executable-stack /usr/lib64/cinelerra-gg/plugins/themes/theme_bright.plugin
cinelerra-gg.x86_64: W: executable-stack /usr/lib64/cinelerra-gg/plugins/themes/theme_hulk.plugin
cinelerra-gg.x86_64: W: executable-stack /usr/lib64/cinelerra-gg/plugins/themes/theme_neophyte.plugin
cinelerra-gg.x86_64: W: executable-stack /usr/lib64/cinelerra-gg/plugins/themes/theme_pinklady.plugin
cinelerra-gg.x86_64: W: executable-stack /usr/lib64/cinelerra-gg/plugins/themes/theme_suv.plugin
cinelerra-gg.x86_64: W: executable-stack /usr/lib64/cinelerra-gg/plugins/themes/theme_unflat.plugin
cinelerra-gg.x86_64: W: executable-stack /usr/lib64/cinelerra-gg/plugins/video/chromakeyhsv.plugin

executable-stack:
The binary declares the stack as executable. Executable stack is usually an
error as it is only needed if the code contains GCC trampolines or similar
constructs which uses code on the stack. One common source for needlessly
executable stack cases are object files built from assembler files which don't
define a proper .note.GNU-stack section.

This can easily be cleared using `execstack -c` on each affected file, but that assumes that the executable stack flag is actually the result of mere oversight, and not actively in use by the software for any reason. So I wanted to check whether any of the listed files genuinely have need to execute operations from their stack, or if it's safe to unset the executable-stack permission using `execstack -c`? If the code isn't consciously making use of stack execution, then it should be safe to clear it. (Obviously, it would be preferable to make the necessary source changes so that the flag is never set to begin with.)
TagsNo tags attached.

Activities

PhyllisSmith

PhyllisSmith

2019-01-02 18:54

manager   ~0000458

We had no problems related to any changes we made as recommended by Ferdnyc when we did the December monthly builds for all of the distros but we did not personally load them all to check. If a user has trouble with running since they almost always run as non-root (which we rarely do!) they probably would have reported so by now. The removing of unnecessary execute bits must be working, the cin desktop icon must be working, and the execstack was no problem for any of the builds. EXCEPT freebsd was not rebuilt and we do not know if that will be a problem but will fix that iif a user finds a need for a new freeBSD version.

Also, as stated previously Centos 7, still used Python 2 (issue 0000011), so if a user reports a problem, they can send email or easily search on the web to create a soft link and resolve the problem.
ferdnyc

ferdnyc

2018-12-17 03:55

reporter   ~0000242

"Regarding the .desktop file, the Encoding= key is fully deprecated by the current standard. "

(And has been since spec 0.9.5, which dates back to roughly 2005/2006.)

https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-0.9.5.html#deprecated-items
ferdnyc

ferdnyc

2018-12-17 03:41

reporter   ~0000241

Regarding the .desktop file, the Encoding= key is fully deprecated by the current standard.

According to https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s05.html the method for encoding strings for certain locales (though UTF-8 _should_ be the default) would be:

Comment[fr.UTF-8]=Éditeur multimédia
PhyllisSmith

PhyllisSmith

2018-12-17 00:40

manager   ~0000238

Re: (1), replaced COPYING with updated GPL2 after much contemplation. Sticking with the status quo.
Re: (2), "The X- categories ... probably_ no POINT in having it there" -- since we had to modify cin.desktop again anyway, deleted the X-Multimedia part. I forgot we needed Encoding=UTF-8 for the French accent graves in the file.
ferdnyc

ferdnyc

2018-12-15 17:27

reporter   ~0000216

"But for the purposes of our package review — well, _nothing_ is "required", the notification here is merely informational, but we only look at the main LICENSE file itself."

Actually, I take that back somewhat. While we certainly only get _involved_ when the text of the LICENSE file is outdated (policy recommending that we notify the upstream project), we do EXAMINE the license statements of every source file in the tree. Which is how we know that, in the case of Cinelerra-GG (from our rpm .spec file):

========
# The Cinelerra-GG codebase is licensed GPLv2+
# The GREYcstoration plugin is licensed CeCILL v2.0
# The Google SHA1 implementation is licensed BSD 3-clause
# The Neophyte theme is licensed Creative Commons CC-BY 4.0
# The freeverb components and the Tapeworm font are in the public Domain
License: GPLv2+ and CeCILL and BSD and CC-BY and Public Domain
========

...That determination was only possible because the incorporated source files, extracted from other projects (and used in full accordance with their license terms) contained boilerplate license statements. So, as I said earlier, point to consider.
ferdnyc

ferdnyc

2018-12-15 17:20

reporter   ~0000214

Re: (2), The X- categories are permitted to define extensions to the standard list, any X-named category is implicitly valid and legal. So there's certainly no harm in having it there. There's also _probably_ no POINT in having it there, admittedly, but point is there's really no urgency one way or the other, as long as it's listed as X-Multimedia.

As far as the license goes, it's of course ultimately up to the project how the license is applied. IANAL, but my understanding is that the boilerplate is not required for the license to be valid or enforceable, and in fact it's merely a "recommendation" by the FSF that the boilerplate be used. In fact, in the text of GPLv3 they state the recommendation as:

========
How to Apply These Terms to Your New Programs

If you develop a new program, and you want it to be of the greatest possible use to the public, the best way to achieve this is to make it free software which everyone can redistribute and change under these terms.

To do so, attach the following notices to the program. It is safest to attach them to the start of each source file to most effectively state the exclusion of warranty; and each file should have at least the “copyright” line and a pointer to where the full notice is found.

    <one line to give the program's name and a brief idea of what it does.>
    Copyright (C) <year> <name of author>

    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
    the Free Software Foundation, either version 3 of the License, or
    (at your option) any later version.

    This program is distributed in the hope that it will be useful,
    but WITHOUT ANY WARRANTY; without even the implied warranty of
    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    GNU General Public License for more details.

    You should have received a copy of the GNU General Public License
    along with this program. If not, see <http://www.gnu.org/licenses/>.

Also add information on how to contact you by electronic and paper mail.
=======
[From: https://opensource.org/licenses/GPL-3.0 ]

So, it would be sufficient in theory to display merely the copyright and authorship lines.

That being said, it's easy to imagine reasons why it might be beneficial to have the full boilerplate statement in a source file, especially in cases where it ends up getting removed from the context of the project and used in isolation somewhere else (either in accordance with or in violation of the license terms). Ultimately, it is only a header comment, and if someone truly finds it a distraction most advanced code editors have some ability to automatically fold such comment blocks if so configured.

But for the purposes of our package review — well, _nothing_ is "required", the notification here is merely informational, but we only look at the main LICENSE file itself. The code headers are, as I said, entirely the project's choice to deal with in whatever manner you see fit.

As with most such things whenever an undertaking intersects with the law, I'm sure you can find someone who will advise you that any course of action is absolutely required to protect your efforts, and someone else who will claim that the same course of action will completely sabotage those efforts.

"The nice thing about standards is that there are so many of them to choose from."
—Andrew S. Tanenbaum
PhyllisSmith

PhyllisSmith

2018-12-15 13:20

manager   ~0000203

Just a short add on for other people who may be reading this.
We have tested the "noexecstack" additional add on Fedora (obviously since that it was we use!) and Mint18 and Ubuntu16. The big concern for the builds on 16 or so distros at the end of the month is always impact on one of them. FreeBSD is a different animal and again, not an area of much expertise. Slackware can be difficult also so we will probably start the "end of the month builds a day early in case of problems".
PhyllisSmith

PhyllisSmith

2018-12-15 13:11

manager   ~0000201

Thank you again for passing this information along so well documented for my/our understanding.
(1) I am still looking into the GPL license and how to best fix/incorporate it. Hoping to put into a single file instead of every routine because it gets in the way for programming. The monitor display is a resource, just like RAM, and using it up for repetitive information is not helpful.
(2) your desktop patch was applied; but I did miss reading about the X-Multimedia so might have to delete that instead
(3) gg did rework the executable bits being set but in a different manner for now
(4) he fixed the execstack issue by forcing the noexecstack option on the load; it seems that that should already be the default since generally this is usually "needless" and not good practice
The files listed used png's for the themes and opengl for the chromakeyhsv and the source code can not be easily revised.

Any technical requotes above may be technically not quite right.
These improvement suggestions are welcome; it is not our area of expertise and is just something gg was roped into having to do to get the software out there.
ferdnyc

ferdnyc

2018-12-13 22:18

reporter   ~0000177

Ah, shoot. I didn't realize my "#digit" numbering in the original report would be translated into issue references. If anyone is able to edit the report text and remove those links, just so it's less confusing, that'd be appreciated. I'll try to remember not to do that in the future. (Mantis _really_ needs a Preview feature for the report/note editor. *sigh*)
ferdnyc

ferdnyc

2018-12-13 22:01

reporter  

0001-.desktop-file-Fixes-to-pass-validation.patch (933 bytes)
From 70babcf263e6cb6fd4b2ad2113006a98435ee4fd Mon Sep 17 00:00:00 2001
From: "FeRD (Frank Dana)" <[email protected]>
Date: Wed, 12 Dec 2018 17:21:56 -0500
Subject: [PATCH] .desktop file: Fixes to pass validation

Fixes to pass desktop-file-validate
- Remove deprecated Encoding key
- No file extension on Icon
- Use valid Categories or X-extended names
---
 cinelerra-5.1/image/cin.desktop | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/cinelerra-5.1/image/cin.desktop b/cinelerra-5.1/image/cin.desktop
index 181a687..3ccfb7a 100644
--- a/cinelerra-5.1/image/cin.desktop
+++ b/cinelerra-5.1/image/cin.desktop
@@ -2,9 +2,8 @@
 Name=cin
 Comment=MultiMedia Editor
 Comment[fr]=Éditeur multimédia
-Categories=Application;AudioVideo;Multimedia;VideoEditing;
-Encoding=UTF-8
+Categories=AudioVideo;AudioVideoEditing;X-Multimedia;
 Exec=cin
-Icon=cin.svg
+Icon=cin
 Terminal=false
 Type=Application
-- 
2.19.2

Issue History

Date Modified Username Field Change
2018-12-13 22:01 ferdnyc New Issue
2018-12-13 22:01 ferdnyc File Added: 0001-.desktop-file-Fixes-to-pass-validation.patch
2018-12-13 22:15 PhyllisSmith Assigned To => goodguy
2018-12-13 22:15 PhyllisSmith Status new => assigned
2018-12-13 22:18 ferdnyc Note Added: 0000177
2018-12-14 00:33 administrator Description Updated View Revisions
2018-12-14 00:33 administrator Additional Information Updated View Revisions
2018-12-15 13:11 PhyllisSmith Note Added: 0000201
2018-12-15 13:20 PhyllisSmith Note Added: 0000203
2018-12-15 17:20 ferdnyc Note Added: 0000214
2018-12-15 17:27 ferdnyc Note Added: 0000216
2018-12-17 00:40 PhyllisSmith Status assigned => resolved
2018-12-17 00:40 PhyllisSmith Resolution open => fixed
2018-12-17 00:40 PhyllisSmith Note Added: 0000238
2018-12-17 03:41 ferdnyc Status resolved => feedback
2018-12-17 03:41 ferdnyc Resolution fixed => reopened
2018-12-17 03:41 ferdnyc Note Added: 0000241
2018-12-17 03:55 ferdnyc Note Added: 0000242
2018-12-17 03:55 ferdnyc Status feedback => assigned
2019-01-02 18:54 PhyllisSmith Status assigned => closed
2019-01-02 18:54 PhyllisSmith Fixed in Version => 2018-12
2019-01-02 18:54 PhyllisSmith Note Added: 0000458