Skip to content
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

Issue420/get nb key@v5 #495

Merged
merged 3 commits into from
Sep 12, 2024
Merged

Conversation

abouteiller
Copy link
Contributor

@abouteiller abouteiller commented Sep 14, 2023

Import changes from v4.2 to v5.1

#491
closes #420

@abouteiller abouteiller self-assigned this Sep 14, 2023
pmix_key_t)

(cherry picked from commit 7bbae9e)

Add the inclusion of an errata list to the revision history

(cherry picked from commit f6dc2ad)
raffenet
raffenet previously approved these changes Apr 11, 2024
@raffenet
Copy link
Contributor

OK, after I clicked approve I went and double checked the OpenPMIx and see that neither API there uses pmix_key_t. I forget if this was discussed when we proposed the change?

PMIX_EXPORT pmix_status_t PMIx_Get(const pmix_proc_t *proc, const char key[],
                                   const pmix_info_t info[], size_t ninfo,
                                   pmix_value_t **val);
PMIX_EXPORT pmix_status_t PMIx_Get_nb(const pmix_proc_t *proc, const char key[],
                                      const pmix_info_t info[], size_t ninfo,
                                      pmix_value_cbfunc_t cbfunc, void *cbdata);

@rhc54
Copy link
Member

rhc54 commented Apr 12, 2024

Yeah, you can't make it a pmix_key_t as the key parameter can be NULL - and IIRC, the compiler will complain if it is declared as a pmix_key_t in that instance.

@abouteiller
Copy link
Contributor Author

We need to double check what happened with #488 as this is when we voted on it. The way that was understood (see #420) the implementation was already using the pmix_key_t and one of the procedures took it as such.

@abouteiller abouteiller marked this pull request as draft April 12, 2024 14:32
@rhc54
Copy link
Member

rhc54 commented Apr 12, 2024

implementation was already using the pmix_key_t and one of the procedures took it as such.

Correct - and as I stated in #420, we corrected it to both APIs using const char key[]. Otherwise, you cannot pass a NULL argument, which would break some of the rules for PMIx_Get

@abouteiller
Copy link
Contributor Author

I did exactly the wrong fix... Good that we caught that before it went out the door. I'll update #420 and we will have to revote on it, I'll check if that'll delay v4.2 based on our rules but I think it will.

@abouteiller
Copy link
Contributor Author

This will now need to mirror #510

@abouteiller abouteiller marked this pull request as ready for review September 12, 2024 16:20
@raffenet raffenet merged commit 9b66641 into pmix:master Sep 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameter Conflict: PMIx_Get vs PMIx_Get_nb
3 participants