Bug 191144 - [patch] Mk/bsd.options.mk: Add opt_IMPLIES and opt_PREVENTS
Summary: [patch] Mk/bsd.options.mk: Add opt_IMPLIES and opt_PREVENTS
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Ports Framework (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Port Management Team
URL:
Keywords:
Depends on:
Blocks: 202491
  Show dependency treegraph
 
Reported: 2014-06-18 01:16 UTC by Adam Weinberger
Modified: 2015-08-21 12:03 UTC (History)
6 users (show)

See Also:


Attachments
patch (1.12 KB, patch)
2014-06-18 01:17 UTC, Adam Weinberger
no flags Details | Diff
Second version. More iterations, better naming. (1.14 KB, patch)
2014-06-19 14:46 UTC, Adam Weinberger
no flags Details | Diff
enables / prevents (1.14 KB, patch)
2014-09-10 17:23 UTC, Adam Weinberger
no flags Details | Diff
IMPLIES/PREVENTS with check_config (1.99 KB, patch)
2015-03-27 17:29 UTC, Adam Weinberger
no flags Details | Diff
improved wording (2.08 KB, patch)
2015-03-31 16:37 UTC, Adam Weinberger
no flags Details | Diff
IMPLIES/PREVENTS (2.23 KB, patch)
2015-03-31 16:49 UTC, Adam Weinberger
no flags Details | Diff
Optimization for _IMPLIES (1.45 KB, patch)
2015-08-21 06:10 UTC, Hiroki Sato
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Weinberger freebsd_committer 2014-06-18 01:16:51 UTC
The attached patch adds the ability for one option to imply another, and for one option to preclude another.

OPTIONS_DEFINE=    SSL INSTALL_SSL_CERTS OPENSSL GNUTLS
INSTALL_SSL_CERTS_IMPLIES=    SSL
OPENSSL_PRECLUDES=    GNUTLS
GNUTLS_PRECLUDES=     OPENSSL

Is there a better way to do the _PRECLUDE functionality? Either way the _IMPLIES functionality is sorely needed and the implementation here is dead simple.
Comment 1 Adam Weinberger freebsd_committer 2014-06-18 01:17:43 UTC
Created attachment 143890 [details]
patch
Comment 2 Baptiste Daroussin freebsd_committer 2014-06-18 06:33:06 UTC
I really do like this new options!

Instead of PRECLUDES why not _CONFLICTS? as a non native speakers I would have not understood PRECLUDES while CONFLICTS sounds straight forward :)
Comment 3 Mathieu Arnold freebsd_committer 2014-06-18 10:36:44 UTC
I like it a lot too, I had an opt_NEEDS patch around for that too[1].

I think you still needs to iterate more than once in your IMPLIES in case there are FOO -> BAR -> BAZ.

Also, like bapt, I think IMPLIES and PRECLUDES, while very nice english words are too nice, in a too high language register for non english people, NEEDS / CONFLICTS are way more straightforward. (I'm not set on NEEDS/CONFLICTS, they could be other, but as a non native english speaker, I had to think about what PRECLUDES meant)

Also, your PRECLUDES/CONFLICT should maybe be done with a OPTIONS_SINGLE (if one has to be choosen) or OPTIONS_RADIO (if none is ok too) option[2], something like this:

OPTIONS_DEFINE=    SSL INSTALL_SSL_CERTS
INSTALL_SSL_CERTS_IMPLIES=    SSL
OPTIONS_RADIO= SSL
OPTIONS_RADIO_SSL= GNUTLS OPENSSL

1: http://people.freebsd.org/~mat/patches/opt_needs.diff
2: http://www.freebsd.org/doc/en/books/porters-handbook/makefile-options.html#makefile-options-syntax
Comment 4 Adam Weinberger freebsd_committer 2014-06-18 11:39:07 UTC
(In reply to Baptiste Daroussin from comment #2)
> Instead of PRECLUDES why not _CONFLICTS? as a non native speakers I would
> have not understood PRECLUDES while CONFLICTS sounds straight forward :)

We already have _CONFLICTS, so that one is unavailable unfortunately.

What about _PREVENTS?
Comment 5 Adam Weinberger freebsd_committer 2014-06-18 11:47:22 UTC
(In reply to Mathieu Arnold from comment #3)
> I think you still needs to iterate more than once in your IMPLIES in case
> there are FOO -> BAR -> BAZ.

I'll take a look at that. Instead of iterating n times, it'd be ideal maybe to iterate until the OPTIONS list stops changing. I'll look into that.

> Also, like bapt, I think IMPLIES and PRECLUDES, while very nice english
> words are too nice, in a too high language register for non english people,
> NEEDS / CONFLICTS are way more straightforward. (I'm not set on
> NEEDS/CONFLICTS, they could be other, but as a non native english speaker, I
> had to think about what PRECLUDES meant)

PRECLUDES is very accurate but is definitely a difficult word. CONFLICTS is already taken. I think PREVENTS would be a nice option. I personally think IMPLIES is a good choice but NEEDS would work too. For what it's worth, my original patch had IMPLIES/PREVENTS but I changed it to [...]/PRECLUDES at the last minute.

> Also, your PRECLUDES/CONFLICT should maybe be done with a OPTIONS_SINGLE (if
> one has to be choosen) or OPTIONS_RADIO (if none is ok too) option[2],
> something like this:

ports-mgmt/dialog4ports already handles that, though that doesn't prevent you from shooting yourself in the foot with make.conf or command line options. Baking that extra protection in with PREVENTS wouldn't be hard, but is it necessary?
Comment 6 Mathieu Arnold freebsd_committer 2014-06-18 12:07:11 UTC
(In reply to Adam Weinberger from comment #5)
> ports-mgmt/dialog4ports already handles that, though that doesn't prevent
> you from shooting yourself in the foot with make.conf or command line
> options. Baking that extra protection in with PREVENTS wouldn't be hard, but
> is it necessary?

Mmm, I think the check-config target always run at the beginning of things, so you should not be able to shoot yourself too much.
Comment 7 Adam Weinberger freebsd_committer 2014-06-19 14:46:22 UTC
Created attachment 143927 [details]
Second version. More iterations, better naming.

Attaching second version of the patch.

This one iterates 3 times, and makes it easy to change the depth. It also changes "PRECLUDES" to "PREVENTS".
Comment 8 Adam Weinberger freebsd_committer 2014-06-30 13:26:15 UTC
What about _ADDS / _REMOVES?
Comment 9 Adam Weinberger freebsd_committer 2014-07-16 17:23:51 UTC
*poke*
Comment 10 Adam Weinberger freebsd_committer 2014-09-01 15:18:43 UTC
Guys, I still am really interested in this concept.

Can somebody please work with me on this? If it has problems, please help me understand how to make it better.
Comment 11 Mathieu Arnold freebsd_committer 2014-09-10 12:23:15 UTC
(In reply to Adam Weinberger from comment #10)
> Guys, I still am really interested in this concept.
> 
> Can somebody please work with me on this? If it has problems, please help me
> understand how to make it better.

Ah, sorry for dropping the ball on this one.

I think the first loop should be:

.for count in ${PORT_OPTIONS}
.  for opt in ${PORT_OPTIONS}

to catch the there are n options, and n depends on n-1 all the way.
Comment 12 Adam Weinberger freebsd_committer 2014-09-10 17:23:07 UTC
(In reply to Mathieu Arnold from comment #11)
I think the first loop should be:
> 
> .for count in ${PORT_OPTIONS}
> .  for opt in ${PORT_OPTIONS}
> 
> to catch the there are n options, and n depends on n-1 all the way.

Ahh yes, you're spot-on! That's the exact number of iterations needed. That hardcoded count was not sitting well with me.

I've updated the patch, and I changed the names to words that are still clear and precise, but more common words.

  ${opt}_ENABLES
  ${opt}_PREVENTS
Comment 13 Adam Weinberger freebsd_committer 2014-09-10 17:23:37 UTC
Created attachment 147180 [details]
enables / prevents
Comment 14 Adam Weinberger freebsd_committer 2015-03-13 17:59:41 UTC
I'd still like to be able to commit this. Is ENABLES/PREVENTS acceptable word choice?
Comment 15 Antoine Brodin freebsd_committer 2015-03-13 18:17:42 UTC
I don't really like _ENABLES, we already have _ENABLE with a totally different meaning
Comment 16 Antoine Brodin freebsd_committer 2015-03-13 18:19:14 UTC
err _CONFIGURE_ENABLE
Comment 17 Adam Weinberger freebsd_committer 2015-03-13 18:22:26 UTC
I completely agree with you there, Antoine.

What about
    FOO_OPTIONADD
    FOO_OPTIONDELETE

It's got a nice readable clarity to it, and there's no ambiguity with other OPTIONS helpers.
Comment 18 Adam Weinberger freebsd_committer 2015-03-26 22:08:42 UTC
This PR has been alive for 9 months waiting for consensus on the option names. 

My favourite of all the possibilities discussed is:

FOO_IMPLIES
FOO_PREVENTS

GTK_IMPLIES=   X11
GTK_PREVENTS=  QT
QT_IMPLIES=    X11
QT_PREVENTS=   GTK
Comment 19 Mathieu Arnold freebsd_committer 2015-03-26 22:41:48 UTC
Now that I think about it, instead of adding an IGNORE line, extending pre-check-config/_check-config in bsd.port.mk would be better, wouldn't it ?
Comment 20 Mathieu Arnold freebsd_committer 2015-03-26 22:46:02 UTC
(also, I like implies/prevent
Comment 21 Bryan Drewery freebsd_committer 2015-03-26 22:46:49 UTC
+1 to idea.
Comment 22 Mathieu Arnold freebsd_committer 2015-03-26 22:52:36 UTC
the idea with pre-check-config/_check-config is that

pre-check-config would check for incompatibilities with the *_PREVENTS, set a variable, and _check-config would look at that variable and burp :-)
Comment 23 Adam Weinberger freebsd_committer 2015-03-27 17:29:50 UTC
Created attachment 154870 [details]
IMPLIES/PREVENTS with check_config

Attached patch:
  - Uses IMPLIES and PREVENTS nomenclature
  - Leverages pre-check-config and _check_config to detect conflicts at config-time

If there are multiple conflicts, the error message looks like:
====> Option FOO conflicts with BAZ BOP. You may select only one of them
Comment 24 Mathieu Arnold freebsd_committer 2015-03-30 20:41:02 UTC
Looks good to me, maybe something different for the last bit:

.if defined(OPTIONS_WRONG_PREVENTS)
	@${ECHO_MSG} "====> You may select only one of:"
.for prevents in ${OPTIONS_WRONG_PREVENTS}
	@${ECHO_MSG} "=====> Option ${prevents} conflicts with ${OPTIONS_WRONG_PREVENTS_${prevents}}."
.endfor
.endif

or something, but it's details.
Comment 25 Adam Weinberger freebsd_committer 2015-03-31 16:36:13 UTC
How's this:

====> Two or more enabled options conflict with each other
=====> Option FOO conflicts with BAR BAZ (select only one)
=====> Option RUNS_HOUSE conflicts with WHOSE_HOUSE (select only one)
Comment 26 Adam Weinberger freebsd_committer 2015-03-31 16:37:05 UTC
Created attachment 155061 [details]
improved wording

Patch with improved wording.
Comment 27 Bryan Drewery freebsd_committer 2015-03-31 16:40:31 UTC
(In reply to Adam Weinberger from comment #26)
> Created attachment 155061 [details]
> improved wording
> 
> Patch with improved wording.

_PREVENTS needs to set _CHECK_CONFIG_ERROR. If the maintainer knows the
options conflicts and is either broken at build or at run, then don't try building. Keep it fatal like the IGNORE was doing.
Comment 28 Adam Weinberger freebsd_committer 2015-03-31 16:49:06 UTC
Created attachment 155062 [details]
IMPLIES/PREVENTS

Ahh yes good point. Attached new patch that sets _CHECK_CONFIG_ERROR.
Comment 29 Bryan Drewery freebsd_committer 2015-03-31 16:51:23 UTC
I'm tempted to just commit this, but my intuition is that _IMPLIES may be confusing since the 'make config' box does not show this fact. There's no other indication the option was forced on. Someone may wonder why they keep getting an option forced when they did not pick it.

I haven't actually tested this so please correct me if I'm wrong about something.
Comment 30 Bryan Drewery freebsd_committer 2015-03-31 16:59:49 UTC
Perhaps _IMPLIES should be _REQUIRES and should be fatal.
Comment 31 Baptiste Daroussin freebsd_committer 2015-03-31 17:20:10 UTC
If that is the final design then I do think we should just modify dialog4ports to support that
I am adding the author in CC so that he can argue/work on this.

In that case first dialog4ports should get updated, then the framework
Comment 32 Adam Weinberger freebsd_committer 2015-03-31 17:26:18 UTC
One option would be to print a message after config that says:

====> Option FOO automatically enables option(s) BAZ BOP

Though that will be printed every time make is run.


That being said, having opt_MSG that shows a line at the bottom of the dialogue would be marvelous.
Comment 33 m1cro 2015-04-01 13:53:57 UTC
Hi All,

Adam,
> That being said, having opt_MSG that shows a line at the bottom of the dialogue would be marvelous.

You mean use help bar? Replace /Detailed help is available hit F1 or ^E to view it/ at some time for other message?
Comment 34 Adam Weinberger freebsd_committer 2015-04-01 13:57:55 UTC
Yes, or a line above/below it. Having a one-line that you can guarantee is always visible would be helpful all over the place. Should we open a separate PR for this?
Comment 35 m1cro 2015-04-01 14:36:22 UTC
Yes, please.
One question:
Is PREVENTS works like RADIO/SINGLE? Maybe no need to do it.
Comment 36 Mathieu Arnold freebsd_committer 2015-04-01 14:50:26 UTC
> Is PREVENTS works like RADIO/SINGLE? Maybe no need to do it.

Yes, and no, because, for instance, you could have:


OPTIONS_DEFINE= PORTS_SSL
OPTIONS_SINGLE= GSSAPI
OPTIONS_SINGLE_GSSAPI=	GSSAPI_BASE GSSAPI_HEIMDAL GSSAPI_MIT GSSAPI_NONE

and then, say:

GSSAPI_BASE_PREVENTS= PORTS_SSL

because, it would end up linking with both libcrypto.so from base and ports and would conflict.
Comment 37 m1cro 2015-04-01 15:16:38 UTC
Oh, yes, you are right.
Comment 38 Mathieu Arnold freebsd_committer 2015-04-01 15:24:51 UTC
But I think what was being discussed was adding IMPLIES support to dialog4ports, not PREVENTS, PREVENTS will already tell you you did wrong and ask you to correct the options.
Comment 39 Ben Woods freebsd_committer 2015-04-08 10:16:46 UTC
(In reply to Adam Weinberger from comment #25)
> =====> Option RUNS_HOUSE conflicts with WHOSE_HOUSE (select only one)

"Say what"???

Ok, enough rapping. My opinion (for what it's worth):

Port options are not always set/unset using the "make config" dialog4ports method. Often they are set/unset in make.conf (in the case of poudriere). This solution needs to be robust to both methods.

Therefore I believe a _CHECK_CONFIG_ERROR should be thrown in either scenario where the requirements are not met, rather than automatically modifying the set/unset options to satisfy the requirements.

This would probably alter the terminology you would use for the option naming:
 - IMPLIES/PREVENTS gives the impression the requirements will be automatically satisfied
 - REQUIRES/CONFLICTS gives the impression an error will be thrown if the requirements are not satisfied

I believe that the OPT1_CONFLICTS=OPT2 terminology could be used for this purpose even though we already have a CONFLICTS=pkg settings. This setting is about option conflicts, whilst other setting is about package conflicts (but they are both conflicts in their own right). I also note that in your explanation of the PREVENTS terminology in your patch, you have used the word conflicts - meaning it is an obvious answer.

Lastly, I believe this patch could be committed prior to any changes to dialog4ports. Having the dialog4ports interface prompt you that something unexpected is going to occur is definitely nice, but not strictly required (particularly in the case where options are set/unset from make.conf for poudriere). Suggest we could commit this now and work on the dialog4ports improvements later to improve the interface (especially considering that no ports use this feauture yet).
Comment 40 Ben Woods freebsd_committer 2015-04-08 10:20:17 UTC
I forgot to say that I think this is a great idea, and look forward to this feature. Thanks Adam!
Comment 41 Mathieu Arnold freebsd_committer 2015-04-08 11:33:03 UTC
Yeah, while dialog4ports support would be a plus, this should go as is.
Comment 42 Mathieu Arnold freebsd_committer 2015-04-08 11:52:37 UTC
(In reply to Ben Woods from comment #39)

Also, opt_CONFLICTS already exists, it sets CONFLICTS if the option is set :-)
Comment 43 Ben Woods freebsd_committer 2015-04-13 14:39:41 UTC
(In reply to Mathieu Arnold from comment #42)
> Also, opt_CONFLICTS already exists, it sets CONFLICTS if the option is set :-)

Oh, of course... I missed that. Also, I know that ports tree is soon to grow the provides/requires functionality, which may mean we cannot use OPT_REQUIRES. So the currently proposed IMPLIES/PREVENTS is probably the best option.

Adam: what are your thoughts on whether IMPLIES should be fatal and throw _CHECK_CONFIG_ERROR, or whether a message should be printed explaining that option FOO has automatically enabled option BAR? I am thinking of the scenario where people have specifically UNSET options in their make.conf or unselected them in make config.
Comment 44 Mathieu Arnold freebsd_committer 2015-06-13 03:09:08 UTC
This has gone on long enough, I think.  Adam, could you check if the patch still works, if so, can you commit it ?
Comment 45 commit-hook freebsd_committer 2015-08-18 11:01:56 UTC
A commit references this bug:

Author: mat
Date: Tue Aug 18 11:00:59 UTC 2015
New revision: 394573
URL: https://svnweb.freebsd.org/changeset/ports/394573

Log:
  Introduce <opt>_IMPLIES and <opt>_PREVENTS to register dependencies, or
  conflicts, between options.

  PR:		191144
  Submitted by:	adamw
  Sponsored by:	Absolight

Changes:
  head/CHANGES
  head/Mk/bsd.options.mk
  head/Mk/bsd.port.mk
Comment 46 Mathieu Arnold freebsd_committer 2015-08-18 11:02:17 UTC
Close this.
Comment 47 Hiroki Sato freebsd_committer 2015-08-21 06:10:45 UTC
Created attachment 160162 [details]
Optimization for _IMPLIES

The nested loop for _IMPLIES in r394573 is too heavy because it takes O(n^2) even in the best case (and O(n^n) in the worst).  As some pointed out on -ports@, 100 options make a user-visible difference on a powerful box...

Another problem is that the outer iteration should be done in the number of COMPLETE_OPTIONS_LIST, not PORT_OPTIONS, because a single FOO_IMPLIES can add multiple options.

I created a patch to solve the above two.  Can anyone review it?  The primary difference is that iteration becomes O(n) in the best case.  I tested it briefly and confirmed that if worked much faster in a deeply-nested case.
Comment 48 commit-hook freebsd_committer 2015-08-21 12:03:13 UTC
A commit references this bug:

Author: mat
Date: Fri Aug 21 12:02:51 UTC 2015
New revision: 394939
URL: https://svnweb.freebsd.org/changeset/ports/394939

Log:
  Optimize opt_IMPLIES.

  PR:		191144
  Submitted by:	hrs
  Sponsored by:	Absolight

Changes:
  head/Mk/bsd.options.mk
Comment 49 Mathieu Arnold freebsd_committer 2015-08-21 12:03:32 UTC
Thanks, I was working on something similar, but yours look better.