Bug 191144
| Summary: | [patch] Mk/bsd.options.mk: Add opt_IMPLIES and opt_PREVENTS | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Adam Weinberger <adamw> | ||||||||||||||||
| Component: | Ports Framework | Assignee: | Port Management Team <portmgr> | ||||||||||||||||
| Status: | Closed FIXED | ||||||||||||||||||
| Severity: | Affects Only Me | CC: | bapt, bdrewery, hrs, mat, rum1cro, woodsb02 | ||||||||||||||||
| Priority: | --- | ||||||||||||||||||
| Version: | Latest | ||||||||||||||||||
| Hardware: | Any | ||||||||||||||||||
| OS: | Any | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 202491 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Adam Weinberger
2014-06-18 01:16:51 UTC
Created attachment 143890 [details]
patch
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 :) 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 (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? (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? (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. 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".
What about _ADDS / _REMOVES? *poke* 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. (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. (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 Created attachment 147180 [details]
enables / prevents
I'd still like to be able to commit this. Is ENABLES/PREVENTS acceptable word choice? I don't really like _ENABLES, we already have _ENABLE with a totally different meaning err _CONFIGURE_ENABLE 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.
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 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 ? (also, I like implies/prevent +1 to idea. 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 :-) 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
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.
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) Created attachment 155061 [details]
improved wording
Patch with improved wording.
(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. Created attachment 155062 [details]
IMPLIES/PREVENTS
Ahh yes good point. Attached new patch that sets _CHECK_CONFIG_ERROR.
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. Perhaps _IMPLIES should be _REQUIRES and should be fatal. 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 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. 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?
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? Yes, please. One question: Is PREVENTS works like RADIO/SINGLE? Maybe no need to do it. > 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.
Oh, yes, you are right. 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. (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). I forgot to say that I think this is a great idea, and look forward to this feature. Thanks Adam! Yeah, while dialog4ports support would be a plus, this should go as is. (In reply to Ben Woods from comment #39) Also, opt_CONFLICTS already exists, it sets CONFLICTS if the option is set :-) (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. This has gone on long enough, I think. Adam, could you check if the patch still works, if so, can you commit it ? 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 Close this. 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.
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 Thanks, I was working on something similar, but yours look better. |