Bug 230916

Summary: [patch] x11-wm/ccsm: add LICENSE and option NLS
Product: Ports & Packages Reporter: Samy Mahmoudi <samy.mahmoudi>
Component: Individual Port(s)Assignee: freebsd-ports-bugs mailing list <ports-bugs>
Status: Open ---    
Severity: Affects Only Me CC: freebsd-ports, ndowens
Priority: --- Keywords: needs-patch, needs-qa
Version: LatestFlags: bugzilla: maintainer-feedback? (freebsd-ports)
Hardware: Any   
OS: Any   
Description Flags
Patch file generated with svn diff
Patch file generated with svn diff
Poudriere log with NLS on
Poudriere log with NLS off
Patch file generated with svn diff none

Description Samy Mahmoudi 2018-08-26 15:29:04 UTC
Created attachment 196569 [details]
Patch file generated with svn diff


• Add option NLS
Comment 1 Nathan 2018-08-26 21:46:52 UTC
is a bit out-of-line of the other options
Comment 2 Samy Mahmoudi 2018-08-27 08:44:46 UTC
Indeed, two paragraphs are indented differently from the rest of the text. This is due to "NLS_CONFIGURE_ENABLE=" being too long to indent "nls" on the first level. I thought indenting the options on a second level would improve readability. We could also have reindented the whole text on this second level. 

Widescreen monitors are rampant. However, sticking to column limits may still be relevant:

• for the audience of users with non-widescreen/standard monitors
• in case of side-by-side diff
• in other cases of multiple panels per screen

What do you suggest for that Makefile ? And in general ?
Comment 3 Tobias Kortkamp freebsd_committer 2018-11-11 13:43:39 UTC
The indentation looks fine to me after applying the diff.

+NLS_USES=		gettext-runtime gettext-tools

This is equivalent to

NLS_USES=	gettext

The patch looks incomplete to me.  It either doesn't do much or is
missing changes to pkg-plist to stop installing the locale files.
Please confirm that it builds with with NLS=on and NLS=off in
Comment 4 Tobias Kortkamp freebsd_committer 2019-01-21 08:57:37 UTC
Actually partially landed now without the NLS changes.
Comment 5 commit-hook freebsd_committer 2019-01-21 08:58:03 UTC
A commit references this bug:

Author: tobik
Date: Mon Jan 21 08:57:15 UTC 2019
New revision: 490835
URL: https://svnweb.freebsd.org/changeset/ports/490835

  x11-wm/ccsm: Add LICENSE

  - While here add missing USES=gnome
  - Reset maintainer due to the long timeout

  PR:		230916
  Submitted by:	Samy Mahmoudi <samy.mahmoudi@gmail.com>
  Approved by:	freebsd-ports@dan.me.uk (maintainer timeout, ~5 months)

Comment 6 Samy Mahmoudi 2019-02-13 01:24:49 UTC
(In reply to Tobias Kortkamp from comment #4)

Hi Tobias!

Thank you for your review and your commit.
Comment 7 Samy Mahmoudi 2019-02-13 02:20:47 UTC
Created attachment 201971 [details]
Patch file generated with svn diff

(In reply to Tobias Kortkamp from comment #3)

Thank you for pointing this out.

• While splitting gettext into gettext-runtime and gettext-tools clearly allows more control in general, it does not add much here. However, the FreeBSD Porter's Handbook now describes the more-concise-but-not-explicit gettext as being "deprecated": https://www.freebsd.org/doc/en/books/porters-handbook/uses-gettext.html

• As you mentioned, the patch was incomplete. I followed your advice and changed pkg-plist accordingly. I also made additional changes to drop gettext and intltool dependencies when NLS is off.
Comment 8 Samy Mahmoudi 2019-02-13 02:23:47 UTC
Created attachment 201972 [details]
Poudriere log with NLS on
Comment 9 Samy Mahmoudi 2019-02-13 02:24:18 UTC
Created attachment 201973 [details]
Poudriere log with NLS off
Comment 10 Tobias Kortkamp freebsd_committer 2019-02-13 09:33:33 UTC
Comment 11 Tobias Kortkamp freebsd_committer 2019-02-23 04:09:02 UTC
Comment on attachment 201971 [details]
Patch file generated with svn diff

Kudos for making it work, but this is adds 8 new patches.  Is it really worth it? 
Do you plan to upstream the changes (assuming they are active)?
Comment 12 Samy Mahmoudi 2019-02-24 04:04:11 UTC
Created attachment 202311 [details]
Patch file generated with svn diff

(In reply to Tobias Kortkamp from comment #11)

> Kudos for making it work, but this is adds 8 new patches.  Is it really worth it?
I dropped 7 patches with a post-patch definition and a for loop ;-)

More seriously, this NLS option was initially suggested by portlint while I was keeping all the compiz ports up-to-date. Thereafter, it seemed worth to me to offer option NLS for users who globally set NLS to off by adding OPTIONS_UNSET+=NLS to their make.conf, even though I am not one of these (I am a French native speaker).

> Do you plan to upstream the changes (assuming they are active)?
I do not because compiz is dead (to such a point that sources are currently fetched from the FreeBSD distribution cache). There is, however, a community around compiz-reloaded and I have already submitted changes to the build system to increase portability and eventually port compiz-reloaded to FreeBSD (https://gitlab.com/compiz/compiz-core/merge_requests/121). I think they do not have the time to review my work, so I am seriously thinking to create a new port with the changes done within the FreeBSD port system and to resubmit the changes upstream, independently, in small chunks to ease in review.

So my goal is to port compiz-reloaded to FreeBSD. During that transition and before considering any drop, we need to keep the compiz ports in the ports tree, preferably in a clean state.