Bug 241216 - x11-wm/compton: grab and update to 7.4
Summary: x11-wm/compton: grab and update to 7.4
Status: In Progress
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Jan Beich
URL:
Keywords: patch, patch-ready
Depends on:
Blocks:
 
Reported: 2019-10-12 13:54 UTC by Jan Beich
Modified: 2019-10-23 18:37 UTC (History)
4 users (show)

See Also:
bugzilla: maintainer-feedback? (femc7488)
jsm: maintainer-feedback+
jbeich: maintainer-feedback? (jt)


Attachments
v1 (11.47 KB, patch)
2019-10-12 13:54 UTC, Jan Beich
no flags Details | Diff
v1.1 (12.16 KB, patch)
2019-10-12 18:23 UTC, Jan Beich
no flags Details | Diff
v1.2 (12.16 KB, patch)
2019-10-23 18:37 UTC, Jan Beich
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Beich freebsd_committer 2019-10-12 13:54:43 UTC
Created attachment 208266 [details]
v1

- Switch to new upstream
- Drop redundant option descriptions
- Drop redundant DOCS dependencies
- Connect tests to the framework
- Respect PREFIX != LOCALBASE for compton.conf
Comment 1 Jan Beich freebsd_committer 2019-10-12 13:55:16 UTC
Alexey, are you de facto maintainer of this port? If so review would be appreciated but, as usual, subject to timeouts.
Comment 2 Jan Beich freebsd_committer 2019-10-12 14:13:26 UTC
Jesper, can you check if x11/compton-conf still works?
JT, can you check if x11/lumina-core still works?
Comment 3 Alexey Dokuchaev freebsd_committer 2019-10-12 14:38:46 UTC
I'm not just de-facto maintainer, I actually use this port.  Given the size of the proposed patch and amount of changes, please give me some time to properly review it.
Comment 4 Jesper Schmitz Mouridsen freebsd_committer 2019-10-12 16:21:38 UTC
(In reply to Jan Beich from comment #2)
Well some compton-config names has been deprecated, and some set by
compton-conf are already gone, but after a short test the most relevant settings have retained their names, or have sane defaults. If you take a test your self, keep in mind that you must restart compton after applying settings in compton-conf. See https://github.com/lxqt/compton-conf/issues/18
Comment 5 Jan Beich freebsd_committer 2019-10-12 18:23:09 UTC
Created attachment 208270 [details]
v1.1

Cosmetic changes (TIMESTAMP, WWW, patch comments, commit message)
Comment 6 q5sys 2019-10-13 17:33:04 UTC
(In reply to Jan Beich from comment #2)
I dont think there should be any issues on the Lumina-Core front.
Comment 7 Jan Beich freebsd_committer 2019-10-19 12:19:14 UTC
(In reply to Alexey Dokuchaev from comment #3)
> I'm not just de-facto maintainer, I actually use this port.

I also use this port and have been for years (but maybe not as long as you). Unfortunately, MAINTAINER field doesn't support more than 1 email and FreeBSD.org bugzilla instance doesn't support generic review flag.

> Given the size of the proposed patch and amount of changes

Most risky changes come from upstream, the rest can be reviewed in ~10 minutes (ignoring time on rationalizing stuff you don't like). According to https://repology.org/project/compton/versions many Linux distributions have switched yshui's fork, even NetBSD. For one, "--vsync none" no longer lies after v5 per https://github.com/yshui/compton/commit/e2182bb00bb4 which made me realize my config relied on a buggy behavior. ;)

> please give me some time to properly review it.

Did you miss "subject to timeouts" in comment 1? Make sure to provide some feedback by the next Saturday. Blatant stalling for time will be ignored as "cookie licking".
Comment 8 Alexey Dokuchaev freebsd_committer 2019-10-23 16:42:27 UTC
> According to [Repology] many Linux distributions have switched yshui's
> fork, even NetBSD.
Fair enough.  It would be nice if we could avoid the PORTEPOCH bump but I guess we cannot. :(

> (ignoring time on rationalizing stuff you don't like)
Well, that's a bit dismissive being said upfront.  Anyway, the patch looks good for the most parts, except for couple of nits below:

> - Drop redundant option descriptions
Actually, as much as I'm a fan of default option descriptions, I felt that they were not descriptive enough for this port, e.g. see ports r433464 for DRM and OpenGL.  Same for PCRE support: default wording ("Use ...") does not really explain what kind of "use", but if now it spans beyond the blacklisting, I guess we can live with default generic, vague PCRE_DESC.

> - Connect tests to the framework
Having tests is nice, but I don't understand the point of leaking the framework-level stuff to user-exposed OPTIONS.  Why not simply hook the tests under TEST_TARGET ("make test") and remove the option?

> Did you miss "subject to timeouts" in comment 1?
No I didn't, why?

> Blatant stalling for time will be ignored as "cookie licking".
I'm not sure what that's supposed to mean, but it sounds rude and insulting.  Go lick your "cookie" yourself.
Comment 9 Jan Beich freebsd_committer 2019-10-23 18:37:51 UTC
Created attachment 208534 [details]
v1.2

Changes since previous version:
- Disable DRM option  by default per startup warning
  [ 10/23/19 18:26:27.091 vsync_init WARN ] The DRM vsync method is deprecated, please don't enable it.
- Make TEST option unconditional

(In reply to Alexey Dokuchaev from comment #8)
>> - Drop redundant option descriptions
> Actually, as much as I'm a fan of default option descriptions, I
> felt that they were not descriptive enough for this port, e.g. see
> ports r433464 for DRM and OpenGL.  Same for PCRE support: default
> wording ("Use ...") does not really explain what kind of "use", but
> if now it spans beyond the blacklisting, I guess we can live with
> default generic, vague PCRE_DESC.

Current OPENGL_DESC is as ambiguous as upstream. "VSync method" is no longer supported as --vsync is now boolean while "backend" is just an implementation detail for "2D rendering the whole screen".

  option('opengl', type: 'boolean', value: true, description: 'Enable features that require opengl (opengl backend, and opengl vsync methods)')

Users are supposed to refer to documentation to figure out where and how OpenGL, PCRE, etc. are used. Trying to tweak options *before* familarizing with the software is footshooting.

>> - Connect tests to the framework
> Having tests is nice, but I don't understand the point of leaking
> the framework-level stuff to user-exposed OPTIONS.  Why not simply
> hook the tests under TEST_TARGET ("make test") and remove the
> option?

`test` was part of non-default option in order to avoid `-Dunittest=true`. Given `--unittest` is harmless if unused always building it seems OK.