Bug 240526 - x11/xlockmore: fix XLOCKLIBS if both MESAGL & KERBEROS5 options set (also KERBEROS5 is broken)
Summary: x11/xlockmore: fix XLOCKLIBS if both MESAGL & KERBEROS5 options set (also KER...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Jason Helfman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-12 09:04 UTC by John Hein
Modified: 2019-10-04 20:02 UTC (History)
2 users (show)

See Also:
jgh: maintainer-feedback-


Attachments
[patch] allow multiple values for XLOCKLIBS, fix KERBEROS5 (1.43 KB, patch)
2019-09-12 09:04 UTC, John Hein
jcfyecrayz: maintainer-approval? (jgh)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Hein 2019-09-12 09:04:08 UTC
Created attachment 207409 [details]
[patch] allow multiple values for XLOCKLIBS, fix KERBEROS5

If both MESAGL & KERBEROS5 options set, XLOCKLIBS is defined separately twice in CONFIGURE_ENV (the last one winning).

Also, KERBEROS5 was broken.  If you build with KERBEROS5 option and either KRB5_HOME=/usr/local and security/krb5 installed OR with KRB5_HOME=/usr, you get a link error:

../xlock/passwd.o: In function `checkPasswd':
passwd.c:(.text+0x8f): undefined reference to `krb5_get_init_creds_opt_init'
passwd.c:(.text+0x9b): undefined reference to `krb5_init_context'
passwd.c:(.text+0xb5): undefined reference to `com_err'
passwd.c:(.text+0xdb): undefined reference to `krb5_parse_name'
passwd.c:(.text+0x100): undefined reference to `krb5_cc_default'
passwd.c:(.text+0x11a): undefined reference to `com_err'
 .
 .

Finally, USE_GL by itself is deprecated:

============
Using USE_GL alone is deprecated, please add USES=gl.

USE_GL=yes is deprecated, please add USE_GL=glu (default) or specify component
============

The attached patch addresses these issues:
 - Fix USES/USE_GL for gl components (MESAGL)
 - Fix kerberos libs (link error) & add USES=gssapi:mit (KERBEROS5) [1]
 - Fix XLOCKLIBS when both MESAGL & KERBEROS5 are selected

[1] Instead of USES=gssapi:mit in the patch, we could provide an option for selecting which kerberos implementation to use.  Other ports provide a radio choice for the other kerberos options (mit, heimdal, base) - e.g., see net/wireshark and net/samba410.  Allowing a kerberos option choice could be added later.

QA:
  poudriere testport with MESAGL & KERBEROS5 set: ok
  portlint: patch fixes a previous portlint warning [2]

[2]
WARN: Makefile: "BROKEN" has to appear earlier.
Comment 1 Jason Helfman freebsd_committer 2019-09-12 17:19:04 UTC
I agree with all changes, however after looking at other ports I found that checking for KRB5_HOME wasn't done in most ports I researched. I believe this is the wrong approach. I was aware of the BROKEN bit, however this is an issue with portlint, and IMHO portlint should be updated to be more flexible where BROKEN is defined.

I am trying to address the case where someone selects the KERBEROS5 option, but doesn't define KRB5_HOME, or other applicable check to make the build work for enabling this option. 

Can you please update this patch with a proper check for the option being enabled?

Similar work is being done here: https://reviews.freebsd.org/D21556

Perhaps this issue can be closed, and we can work on it in the above location?

Thanks for your patch! I would be happy to implement a fix, but I would like a proper test done for the option, unless I am missing something and that is done.
Comment 2 John Hein 2019-09-13 10:39:17 UTC
I didn't remove the KRB5_HOME check because of a portlint warning.  I removed it because the addition of USES=gssapi makes it not necessary.  The patch I added solves the 'select KERBEROS5 without defining KRB5_HOME' issue you describe.

The reviews link you provided identifies different problems than this bug.  But I looked into some of it, and it seems at least partly wrong to me.  For instance, removing the GTK2 option is not necessary.  The problem with the GTK2 option is the malformed files/patch-xglock_xglock.c - that patch should just be removed because what it was trying to fix is now incorporated in 5.58.  And removing KERBEROS5 option is not necessary with the attachment 207409 [details] patch.

I can comment in the review, but after removing the changes for GTK2 and KERBEROS5, it doesn't leave much of value.  The XINERAMA_USES changes seem fine.  Maybe the MB changes, but I didn't look too closely at it.  Same with the setuid issue - that could be correct.  I'll try to verify the good parts of that patch and merge with this one unless someone beats me to it.
Comment 3 John Hein 2019-09-13 11:31:07 UTC
In addition to removing the vestigial patch file, the GTK2 option needs this as well:

GTK2_USE= pkgconfig

The configure script uses pkg-config to detect presence/version of the gtk dependency.
Comment 4 Joseph Mingrone freebsd_committer 2019-09-19 02:14:06 UTC
https://reviews.freebsd.org/D21556 author here.

1. Fixing the GTK2/Kerberos5 options is better than removing it as I did.  (I did not have enough time to investigate the failures more deeply, but since they were broken, I did what I thought to be the the least worst thing by removing them in the short term).  I will incorporate the fixes in Phab. revision.

2. The review does fix a few other problems though.  I detail those in the comments of the review.

TODO: I hope to find time in the next few days to handle the different kerberos implementations.

Installing bin/xlock and bin/play.sh appears to be intentional base on '@mode 4111' in pkg-plist and run-time tests.  It might take more work to 'fix' this.
Comment 5 Joseph Mingrone freebsd_committer 2019-09-19 19:47:26 UTC
https://reviews.freebsd.org/D21556 is ready for another look when you have time.
Comment 6 Jason Helfman freebsd_committer 2019-10-04 20:02:43 UTC
This was addressed in r513774.