Skip to content

Specify which VM model to enable via an = option#117

Merged
kostis merged 2 commits intomasterfrom
ARGO-VM-choice
Jun 23, 2022
Merged

Specify which VM model to enable via an = option#117
kostis merged 2 commits intomasterfrom
ARGO-VM-choice

Conversation

@kostis
Copy link
Copy Markdown
Contributor

@kostis kostis commented Jun 16, 2022

Instead of employing -DARGO_VM_??? options for VM management
(and have complicated code to check that exactly one of them is enabled).

Besides simplifying the code, this will enable testing all options more
easily on GitHub actions.

For the moment, the same default has been kept, but it will need to be
revised when #110 is appropriately fixed.

Instead of employing `-DARGO_VM_???` options for VM management
(and have complicated code to check that exactly one of them is enabled).

Besides simplifying the code, this will enable testing all options more
easily on GitHub actions.

For the moment, the same default has been kept, but it will need to be
revised when #110 is appropriately fixed.
@davidklaftenegger
Copy link
Copy Markdown
Member

No strong objection.

untested:

set(ARGO_VM_OPTIONS ANONYMOUS MEMFD SHM )
set(ARGO_VM SHM CACHE STRING "Select how to handle virtual addresses")
set_property(CACHE ARGO_VM PROPERTY STRINGS ${ARGO_VM_OPTIONS})
if(NOT ARGO_VM IN_LIST ARGO_VM_OPTIONS)
    message(FATAL_ERROR "ARGO_VM must be one of ${ARGO_VM_OPTIONS}")
endif()

something like this could be used to restrict the string to valid/expected values.
Do we want that, or is it okay that users have to type the string without typo in the command line or their cmake UI of choice to get the behaviour they want?

@kostis
Copy link
Copy Markdown
Contributor Author

kostis commented Jun 17, 2022

No strong objection.

What does this comment mean? That you only have a weak objection? If so, it is better to explicitly state it than having us guess what this objection might be.

Note that the code in the PR checks for two of the values (currently MEMFD and ANONYMOUS) and if it is not one of them the third value (SHM, which is the current default) is used, even if the user has typed something else. This allows cmake without having to specify something for ARGO_VM and get the default configuration, which is the current behaviour.

Regarding:

something like this could be used to restrict the string to valid/expected values. Do we want that, or is it okay that users have to type the string without typo in the command line or their cmake UI of choice to get the behaviour they want?

IMO, this is an orthogonal issue and something that we can discuss more generally than just in the context of selecting an ARGO_VM model. Note that we can currently happily build the system after a cmake of the form:

$ cmake -DARGO_BACKEND_MPI=YES_PLEASE   -DBUILD_DOCUMENTATION=ALL_OF_IT ../

Why should the ARGO_VM option be special?

@lundgren87
Copy link
Copy Markdown
Member

I agree that it would be good to either:

  • Restrict the possible input space of the ArgoDSM CMake options AND/OR
  • Inform when an invalid option is given and what the outcome is (error / use default)

This concerns all options and could be a separate PR.

@lundgren87
Copy link
Copy Markdown
Member

lundgren87 commented Jun 22, 2022

No strong objection.

untested:

set(ARGO_VM_OPTIONS ANONYMOUS MEMFD SHM )
set(ARGO_VM SHM CACHE STRING "Select how to handle virtual addresses")
set_property(CACHE ARGO_VM PROPERTY STRINGS ${ARGO_VM_OPTIONS})
if(NOT ARGO_VM IN_LIST ARGO_VM_OPTIONS)
    message(FATAL_ERROR "ARGO_VM must be one of ${ARGO_VM_OPTIONS}")
endif()

something like this could be used to restrict the string to valid/expected values. Do we want that, or is it okay that users have to type the string without typo in the command line or their cmake UI of choice to get the behaviour they want?

I think the first three lines of David's suggestion is an elegant solution regardless of validating input (following three lines), as this will present itself as a multiple choice when using ccmake letting the user toggle between the three valid options only.

Here are a few additional examples:
https://www.kitware.com/constraining-values-with-comboboxes-in-cmake-cmake-gui/

@kostis
Copy link
Copy Markdown
Contributor Author

kostis commented Jun 23, 2022

I think the first three lines of David's suggestion is an elegant solution regardless of validating input (following three lines), as this will present itself as a multiple choice when using ccmake letting the user toggle between the three valid options only.

Adopted. But two lines suffice for this.

Copy link
Copy Markdown
Member

@lundgren87 lundgren87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

@kostis kostis merged commit 78e13d5 into master Jun 23, 2022
@kostis kostis deleted the ARGO-VM-choice branch June 23, 2022 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants