Bug 246954

Created attachment 215188 [details]
Update to 2.0.0

Comment 2 Lorenzo Salvadore 2020-06-03 10:45:00 UTC

Thanks Alexander for maintaining hikari.

Are you sure you have not forgotten to include pkg-plist in the patch? I see you removed the PLIST_FILES definition, but no pkg-plist is added.

Created attachment 215189 [details]
Update to 2.0.0 with pkg-plist

I forgot to pass the -N flag to diff.

Comment 4 Lorenzo Salvadore 2020-06-03 12:54:41 UTC

Test with poudriere fails:

====> Running Q/A tests (stage-qa)
====> Checking for pkg-plist issues (check-plist)
===> Parsing plist
===> Checking for items in STAGEDIR missing from pkg-plist
Error: Orphaned: bin/hikari
Error: Orphaned: bin/hikari-unlocker
Error: Orphaned: %%ETCDIR%%/hikari.conf.sample
Error: Orphaned: etc/pam.d/hikari-unlocker
Error: Orphaned: share/backgrounds/hikari/hikari_wallpaper.png
Error: Orphaned: share/man/man1/hikari.1.gz
Error: Orphaned: share/wayland-sessions/hikari.desktop
===> Checking for items in pkg-plist which are not in STAGEDIR
Error: Missing: bin/hikari
Error: Missing: bin/hikari-unlocker
Error: Missing: %%ETCDIR%%/hikari.conf.sample
Error: Missing: etc/pam.d/hikari-unlocker
Error: Missing: share/backgrounds/hikari/hikari_wallpaper.png
Error: Missing: share/man/man1/hikari.1.gz
Error: Missing: share/wayland-sessions/hikari.desktop
===> Error: Plist issues found.
*** Error code 1

Stop.
make: stopped in /usr/ports/x11-wm/hikari
=>> Error: check-plist failures detected

That is a funny error.
My guess is the issue is in the format of your pkg-plist file:
$ file pkg-plist 
pkg-plist: ASCII text, with CRLF line terminators

Please try converting it, for example by using dos2unix from converters/unix2dos.

Moreover, a pair of questions:

- Is files/hikari.desktop.in still needed? I see upstream provides a hikari.desktop file that you are using in do-install.
- Is there a reason you do not use the install target from upstream?

(In reply to Lorenzo Salvadore from comment #4)
This is the second time I'm having a having a problem with a patch being with windows line endings. This must have something to do with the fact that I'm pasting the patch in to the webform and not uploading a file or maybe its a bug in wl-copy. I'll just upload the patches as a file in the future. I'm so sorry this happened.


> - Is files/hikari.desktop.in still needed? I see upstream provides a 
> hikari.desktop file that you are using in do-install.
This is in fact on longer need. I probably just forget another diff options so this deletion gets include in the diff.

> - Is there a reason you do not use the install target from upstream?
The only reason is because upstream set the suid bit on the binary's and this will break the SUID option. I'm currently thinking about just removing the SUID option  because hikari is not much use without suid set on both binary's. What do you think about this?

This is the QA output when using the upstream install target.

I can fix the first two warning with a post-install target, but i'm not sure what to do about the suid warning. Maybe unsetting this in the post-install target and then using the SUID option to set it again.

====> Running Q/A tests (stage-qa)
Warning: 'bin/hikari-unlocker' is not stripped consider trying INSTALL_TARGET=install-strip or using ${STRIP_CMD}
Warning: 'bin/hikari' is not stripped consider trying INSTALL_TARGET=install-strip or using ${STRIP_CMD}
Warning: setuid files in the stage directory (are these necessary?):
874661 -r-sr-xr-x  1 xanderio  wheel  216056 Jun  3 15:49:03 2020 /usr/home/xanderio/Projects/ports/x11-wm/hikari/work/stage/usr/local/bin/hikari
874662 -r-sr-xr-x  1 xanderio  wheel   18736 Jun  3 15:49:03 2020 /usr/home/xanderio/Projects/ports/x11-wm/hikari/work/stage/usr/local/bin/hikari-unlocker

Comment 7 Lorenzo Salvadore 2020-06-03 15:07:59 UTC

Unsetting setuid in stage directory and then set it back through MAYBE_SUID works, but I think the following approach is preferable:

- never set the WITHOUT_SUID variable, so that hikary binary is never installed with setuid in the stage directory;

- patch the Makefile from upstream so that hikari-unlocker is never installed with setuid in the stage directory:
- install -m 4555 hikari-unlocker ${DESTDIR}/${PREFIX}/bin
+ install -m 555 hikari-unlocker ${DESTDIR}/${PREFIX}/bin

Finally set suid if necessary through MAYBE_SUID as you already did.

This approach looks cleaner to me and you have the advantage to never have a setuid file in the stage directory, not even for a fraction of a second, so that users concerned about security feel safier.

Comment 8 Lorenzo Salvadore 2020-06-03 15:22:55 UTC

Actually, I think that approach is not only preferable but should be used for security reasons: we found a way to avoid setuid files in the stage directory even for a fraction of a second and then there is no reason to risk.

Created attachment 215199 [details]
Update to 2.0.0 with request improvements

Comment 10 Lorenzo Salvadore 2020-06-03 17:04:00 UTC

Created attachment 215205 [details]
hikari_3

Thank you very much. I am now testing with a few modifications.

- You defined twice the MAKE_ARGS, always with = instead of +=, so that you were overriding the variable value. I fixed it by defining MAKE_ARGS only once and with +=. Moreover, I have put a blank line after the USE block as MAKE_ARGS is not part of this block (please see https://www.freebsd.org/doc/en/books/porters-handbook/porting-order.html). So these lines become:

USES=           compiler:c11 gnome pkgconfig xorg
USE_GNOME=      cairo glib20 pango
USE_XORG=       pixman

MAKE_ARGS+=      ${WITH_DEBUG:DDEBUG=YES} \
                WITHOUT_SUID=YES

OPTIONS_DEFINE=         GAMMA LAYERSHELL SCREENCOPY SUID X11
OPTIONS_DEFAULT=        GAMMA LAYERSHELL SCREENCOPY SUID X11

- You were stripping binaries without checking WITH_DEBUG before and you forgot to rename hikari.conf to hikari.conf.sample. I fixed the post-install target as follows:

post-install:
.ifndef WITH_DEBUG
        ${STRIP_CMD} ${STAGEDIR}${PREFIX}/bin/hikari
        ${STRIP_CMD} ${STAGEDIR}${PREFIX}/bin/hikari-unlocker
.endif
        ${MV} ${STAGEDIR}${ETCDIR}/hikari.conf \
        ${STAGEDIR}${ETCDIR}/hikari.conf.sample

I have put everything into a patch so that you can check it more easily.

If you are fine with those changes and testing goes fine, as soon as I receive a mentor approval (from gerald or tcberner, who are already CCed in this PR) I will commit your patch.

Thank you very much for maintaining this port. I use it myself and could not wait for 2.0.0 :)

Comment 11 Gerald Pfeifer 2020-06-03 17:15:26 UTC

(In reply to Lorenzo Salvadore from comment #10)
> If you are fine with those changes and testing goes fine, as soon as I
> receive a mentor approval (from gerald or tcberner, who are already CCed
> in this PR) I will commit your patch.

Looks good to me.  Thank you, Alexander, and some really nice refinements,
Lorenzo, and good eyes!

One minor suggestion: In the following, may indent the second line a bit
further so that it cannot be mistaken to be a command invocation (which
would not make sense for a .conf.sample file, plus there is the \ in the
line before, but that one's easy to miss.

+         ${MV} ${STAGEDIR}${ETCDIR}/hikari.conf \
+         ${STAGEDIR}${ETCDIR}/hikari.conf.sample

Comment 12 commit-hook 2020-06-03 19:28:51 UTC

A commit references this bug:

Author: salvadore
Date: Wed Jun  3 19:28:09 UTC 2020
New revision: 537836
URL: https://svnweb.freebsd.org/changeset/ports/537836

Log:
  x11-wm/hikari: Update to 2.0.0

  - Floating views are raised after layout apply.
  - Sheet is reset before layout apply.
  - Focus view is raised to layout on layout apply.
  - Add append/prepend to layout operations.
  - Remove sheet groups (ungrouped views create a group for their app id
    instead).
  - Groups can now start with digits (no more sheet group overlap).
  - Add 'sheet-show-group' operation.
  - Add 'workspace-show-group' operation.
  - Add 'workspace-show-invisible' operation.
  - Add 'workspace-show-all' operation.
  - Add 'workspace-clear' operation.
  - Add 'workspace-show-group' operation.
  - Add 'workspace-cycle-[next|prev]' operations.
  - Add output relative view position configuration (e.g. center,
    bottom-right).
  - Allow tiled views to be moved around.
  - Migrate views to other outputs using move operations (mouse and
    keyboard).
  - Add move libinput configuration options for pointer devices.
  - Add 'ui' section to configuration.
  - Add default configuration file.
  - Add default wallpaper.
  - Many multi-monitor fixes.
  - And many bugfixes.

  Port changes:
  - Use hikari.desktop from upstream.
  - Use install target from upstream: a patch was needed to avoid installing
    setuid files in the stage directory.

  PR:		246954
  Submitted by:	ports@xanderio.de
  Approved by:	gerald (mentor)

Changes:
  head/x11-wm/hikari/Makefile
  head/x11-wm/hikari/distinfo
  head/x11-wm/hikari/files/hikari.desktop.in
  head/x11-wm/hikari/files/patch-Makefile
  head/x11-wm/hikari/pkg-plist

Comment 13 Lorenzo Salvadore 2020-06-03 19:31:21 UTC

Commited, thanks a lot!