Page MenuHomeFreeBSD

Mk: Create public OPTIONS_${section_kind}_${NAME}_SELECTED variables that contain selected multi, group, single, and radio options
Needs ReviewPublic

Authored by yuri on Nov 14 2017, 7:21 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 7, 11:29 PM
Unknown Object (File)
Jan 15 2024, 4:12 PM
Unknown Object (File)
Dec 20 2023, 2:37 AM
Unknown Object (File)
Dec 19 2023, 5:27 PM
Unknown Object (File)
Nov 9 2023, 3:32 AM
Unknown Object (File)
Nov 7 2023, 3:31 AM
Unknown Object (File)
Nov 4 2023, 2:27 PM
Unknown Object (File)
Oct 29 2023, 6:57 PM
Subscribers

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

I don't understand what the benefit of this is. What does this do, and why?

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?

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.

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.

In D13078#273721, @yuri wrote:

@tcberner Who should greenlight this? :)

Sounds like it's up portmgr's road.

@mat , Could you please review this? One port waits for this.

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.

yuri marked 2 inline comments as done.Nov 25 2017, 9:44 PM
yuri added inline comments.
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?

yuri marked 4 inline comments as done.Nov 25 2017, 9:44 PM
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.

Now they are in bsd.options.mk.

yuri retitled this revision from Mk: Create public OPTIONS_SINGLE_${NAME}_SELECTED and OPTIONS_RADIO_${NAME}_SELECTED variables that contain selected single and radio options to Mk: Create public OPTIONS_${section_kind}_${NAME}_SELECTED variables that contain selected single and radio options.Jan 1 2018, 9:20 PM
yuri edited the summary of this revision. (Show Details)
yuri retitled this revision from Mk: Create public OPTIONS_${section_kind}_${NAME}_SELECTED variables that contain selected single and radio options to Mk: Create public OPTIONS_${section_kind}_${NAME}_SELECTED variables that contain selected multi, group, single, and radio options.Jan 2 2018, 7:52 AM

Here are 2 cases that are greatly simplified by this feature:

  1. https://reviews.freebsd.org/D13079
  2. 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:

  1. 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?"
  2. The feature is elegantly implemented in only 9 short lines, not counting comments.

Please take one of these three possible actions:

  1. Approve the patch so that it can be committed
  2. Suggest improvements to the patch
  3. 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.

@portmgr needs to approve it.

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?

In D13078#372447, @yuri wrote:

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?

In D13078#372447, @yuri wrote:

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?

That's what I did (science/simint) for now, thanks for the advise.