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

Need to bump main branch libfabric version #8904

Closed
shijin-aws opened this issue May 8, 2023 · 11 comments
Closed

Need to bump main branch libfabric version #8904

shijin-aws opened this issue May 8, 2023 · 11 comments

Comments

@shijin-aws
Copy link
Contributor

https://github.com/ofiwg/libfabric/blob/main/configure.ac#L11

Should it be updated to 1.19.0 or newer? I am asking because I was seeing a bug caused by this PR #8826 which only populate hmem_data for FI_VERSION >= 1.19, but the current libfabric version is 1.18.0rc1, which is even lower than the v1.18.x branch.

@shijin-aws
Copy link
Contributor Author

@shefty

@shijin-aws shijin-aws changed the title Need to bump libfabric version in configure.ac Need to bump main branch libfabric version in configure.ac May 8, 2023
@shefty
Copy link
Member

shefty commented May 8, 2023

Yes, we can update the version in preparation for the next release.

@shijin-aws
Copy link
Contributor Author

shijin-aws commented May 8, 2023

Yes, we can update the version in preparation for the next release.

Does that mean the version in main branch is always lower than released branch before preparing a release? That sounds odd to me.

The bug in #8826 is that cur_abi_attr->hmem_data = FI_VERSION_GE(user_version, FI_VERSION(1, 19)) is only populated for libfabric version >=1.19, but the mr attr flags OFI_HMEM_DATA_GDRCOPY_HANDLE was added in main branch in https://github.com/ofiwg/libfabric/blob/main/prov/efa/src/efa_mr.c#L268. And here we have an inconsistency.

So we either need to add a FI_VERSION check in https://github.com/ofiwg/libfabric/blob/main/prov/efa/src/efa_mr.c#L268, which is done in #8902,
Or we bump the libfabric version in main branch right now to 1.19.

Which solution do you prefer? I would prefer the latter one as having a main branch of version 1.18* is kind of strange.

@shefty
Copy link
Member

shefty commented May 8, 2023

We updated the main branch to v1.18.0rc1, then created the v1.18.x branch. The main branch simply hasn't been updated with a new version since then. Updating the defines is trivial and can be done. Updating the version in configure.ac is less trivial and either means waiting closer to the actual release or predicting whether there will be any changes in the public header files.

@shijin-aws
Copy link
Contributor Author

or predicting whether there will be any changes in the public header files.

I see, I think we should have updated the version as part of this PR 49d1a76, which changed the public header files.

@shijin-aws
Copy link
Contributor Author

Should we update the version to 1.19.0 or 1.19.0rc1? It looks to me 1.19.0rc1 makes FI_VERSION_GE(user_version, FI_VERSION(1, 19)) returns false as well.

@shefty
Copy link
Member

shefty commented May 8, 2023

The version should be MAJOR 1, MINOR 19, REVISION 0. rc1 is a package number and part of configure.ac.

@shefty
Copy link
Member

shefty commented May 8, 2023

Btw, don't update configure to rc1. You can update to a1 if needed, but I usually update configure as part of the release process.

@shijin-aws
Copy link
Contributor Author

Btw, don't update configure to rc1. You can update to a1 if needed, but I usually update configure as part of the release process.

I see, so I can just bump this line https://github.com/ofiwg/libfabric/blob/main/include/rdma/fabric.h#L87, is that good for you?

@shijin-aws shijin-aws changed the title Need to bump main branch libfabric version in configure.ac Need to bump main branch libfabric version May 8, 2023
@shijin-aws
Copy link
Contributor Author

#8905

@shijin-aws
Copy link
Contributor Author

PR is merged

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

No branches or pull requests

2 participants