Details
- Reviewers
tcberner adamw mat - Group Reviewers
O5: Ports Framework (Owns No Changed Paths) portmgr
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 14043 Build 14231: arc lint + arc unit
Event Timeline
Ah okay now I see what you're getting at. It's very confusing the way you've put it into the comments block. That section is for things that you, the porter, define. But your new variable is something that the options system defines for you. The way it is now, you're implying that people should set that variable manually.
Does this handle options that get changed by IMPLIES? Also, if this is just an alias for "whatever is currently selected", why does it need its own warning block?
How would you reword it then?
Does this handle options that get changed by IMPLIES?
${opt}_IMPLIES isn't affected. Two variables got labeled as 'public' and now can be used by ports, nothing else changed.
Also, if this is just an alias for "whatever is currently selected", why does it need its own warning block?
The warning isn't its own. It is the same warning issued by the framework as before, now based on a public variable.
Once you have a single/radio choice, it is natural to have the variable telling you what is selected.
One example of use is in the linked patch.
| Mk/bsd.port.mk | ||
|---|---|---|
| 4684 ↗ | (On Diff #35219) | If this is part of the options framework and not part of the sanity checking, it should go in bsd.options.mk. |
| 4762–4771 ↗ | (On Diff #35219) | These changes look bogus. |
| Mk/bsd.port.mk | ||
|---|---|---|
| 4684 ↗ | (On Diff #35219) | True, but moving shouldn't be a part of the same patch? |
| 4762–4771 ↗ | (On Diff #35219) | Bogus? I only renamed OPTIONS_WRONG_${FOO}_${BAR} into OPTIONS_${FOO}_${BAR}_SELECTED to make the variable name more generic. Or should I just move all these lines into bsd.options.mk? |
| Mk/bsd.port.mk | ||
|---|---|---|
| 4684 ↗ | (On Diff #35219) | You are missing the point, if you are creating variables that can be used inside a port's Makefile they MUST go in bsd.options.mk. |
| 4762–4771 ↗ | (On Diff #35219) | Please, do not change the check-config code, it works just fine as is. If you want to add some options variable, they MUST go in bsd.options.mk. |
Here are 2 cases that are greatly simplified by this feature:
- https://reviews.freebsd.org/D13079
- https://bugs.freebsd.org/bugzilla/attachment.cgi?id=189569&action=diff
I am more than positive that there are a lot more cases like that that can benefit from this feature.
Additionally, the reasons this feature should be present are:
- It is natural to have a list of selected options out of the list of options offered for selection. The values OPTIONS_${otype}_${section}_SELECTED correspond well to the intuitive expectation expressed in a casual question like "What is selected?"
- The feature is elegantly implemented in only 9 short lines, not counting comments.
Please take one of these three possible actions:
- Approve the patch so that it can be committed
- Suggest improvements to the patch
- Reject the patch with an explanation why this feature should be rejected
Please approve this patch. The gnome-twitch port has been hanging uncommitted for a long time because of this.
I have a port that takes the option that is one of 5 choices. It will use OPTIONS_SINGLE. The name of the selected option needs to be passed as a cmake variable value.
If there is an option section, it is reasonable to have a helper variable that contains all currently selected values. It can simplify certain cases.
The alternative is to have .if/.else/.endif for every choice element.
Can this be approved and committed?
Can't you do something simple like
OPTIONS_SINGLE_FOO= FOO1 FOO2 FOO3 FOO4 FOO5 FOO1_CMAKE_ARGS= -DCMAKE_OPTION:STRING=FOO1 [...] FOO5_CMAKE_ARGS= -DCMAKE_OPTION:STRING=FOO5
in the case you have there, for the time being at least?