Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow one to specify an ABI file via MPIPreferences #600
base: master
Are you sure you want to change the base?
Allow one to specify an ABI file via MPIPreferences #600
Changes from 5 commits
3c1670f
9b7091e
edaf03b
904d252
ff29239
13af812
573ae38
162653a
8022f57
9a5d0a9
b4de3bd
ff798c3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd leave it as
nothing
if it is not set.maybe a slightly longer name (e.g.
abi_source_file
?)also, you need to store it via
set_preferences!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That won't work, since
Nothing
is not a valid TOML type:This is also the reason why used the empty string as a sentinel. I could also use
false
if you'd prefer that.Makes sense, I will do that.
Thanks, I seem to have forgotten to commit that part 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 162653a.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferences.jl supports it, and it is what is returned when value is not set:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i would prefer if this branch was only taken if
abi == "custom"
or something similarThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of the
abi_file
preference as a "one to rule them all" property that just overrides whatever is set as ABI when it comes to loading the ABI file.I do see the merits of your suggestion, though. However, what if users set
abi_file
to something and leaveabi
unset? Or set it to something other thancustom
? Wouldn't it be confusing if theabi_file
is then ignored? Or should this be handled as an error in the call touse_system_binary
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this option will be rarely used (and is untested), I would be extremely reluctant to make it the "one to rule them all".
The configuration is complicated in any case (which is part of the reason I am reluctant to add this functionality), but I would prefer that any requirements be very explicit. The logic in
use_system_binary
should handle these cases.