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

core: 1.8 ABI compat #10385

Merged
merged 1 commit into from
Sep 17, 2024
Merged

core: 1.8 ABI compat #10385

merged 1 commit into from
Sep 17, 2024

Conversation

jiaxiyan
Copy link
Contributor

ABI version is updated to 1.8 to accommodate fi_fabric2() API introduced in #10279

@@ -111,7 +111,7 @@ extern "C" {
* name appended with the ABI version that it is compatible with.
*/

#define CURRENT_ABI "FABRIC_1.7"
#define CURRENT_ABI "FABRIC_1.8"
Copy link
Member

Choose a reason for hiding this comment

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

@j-xiong

This change indicates that the fi_fabric2() call is not aligned with the documentation in this file. Checking the code, it's using DEFAULT_SYMVER(... FABRIC_1.8) directly instead of CURRENT_SYMVER(). There were also changes to fi_getinfo / fi_freeinfo / fi_dupinfo as part of ABI 1.8 changes. The changes to those functions should be captured in the man page below as well.

If CURRENT_SYMVER() will no longer be used, that macro should be removed and the man page updated to reflect direct use of DEFAULT_SYMVER instead.

Copy link
Contributor

@shijin-aws shijin-aws Sep 16, 2024

Choose a reason for hiding this comment

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

@shefty @j-xiong If we are going to implement changes to fi_info today. Is it better to make it part of FABRIC_1.8 (since 2.0 is not released yet?), or we need to bump it as FABRIC_1.9?

Copy link
Contributor

Choose a reason for hiding this comment

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

We will keep it 1.8 for the 2.0 release.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shefty Using DEFAULT_SYMVER() here for the new function instead of CURRENT_SYMVER() has the advantage of showing the exact ABI version at the definition and not needing to change it again when the ABI version is bumped. I think we can remove CURRENT_SYMVER() definition and usage info from the header. If no objection, I will make that change in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to remove CURRENT_SYMVER

ABI version is updated to 1.8 to accommodate fi_fabric2() API.

Signed-off-by: Jessie Yang <[email protected]>
@j-xiong
Copy link
Contributor

j-xiong commented Sep 17, 2024

Intel CI error is unrelated.

@j-xiong j-xiong merged commit 5c96405 into ofiwg:main Sep 17, 2024
12 of 17 checks passed
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.

4 participants