-
Notifications
You must be signed in to change notification settings - Fork 32
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
set symbol versions in def/map files #974
base: v0.10.x
Are you sure you want to change the base?
Conversation
a35d85b
to
aca9a1e
Compare
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.
hmm.. update the RELEASE_STEPS...? what if we have new version with no changes? should we bump (replace) the version? or add a new entry below the current one...?
I think this topic has to be discussed at the UMF tech meeting before we proceed. I agree with @lukaszstolarczuk that it is better not to increase the API version if the API has not changed. |
I think that in such a case you can't just replace the version, you would require that the new library version is linked then. I guess this will require adding (empty?) entry for a new version. But still, this should be described somehow in the RELEASE_STEPS, I agree. |
@vinser52 I thought we already had a little discussion about this. This PR actually fixes the version of symbols to the correct one for all symbols and creates a base for future potential changes. In future releases we would either redefine all symbols or set a new version for diff only - but this would be a separate PR with a separate discussion. @lukaszstolarczuk @PatKamin we already have it - I don't think this require more detailed description. See https://github.com/oneapi-src/unified-memory-framework/blob/main/RELEASE_STEPS.md?plain=1#L46 |
@bratpiorka I think that we should avoid having such a discussion in a PR that will introduce some changes in API. Current info in RELEASE_STEPS suggest to bump the ABI version (all symbols versions bumped), as I understand it. So for now, it looks like the first one to change API in a PR bumps the version of all symbols. |
I have several generic questions that we should agree on:
I do not remember that we discussed such details. We only discussed that we need versioning, but we have not discussed the details. Did I miss something? |
No, we haven't discussed any details yet, that's why this PR is a draft. What my changes do are two things:
about 2 - this needs to be discussed. I will prefer every time we change an existing API + set a new symbols version to the current UMF (e.g. newly added functions in 1.02 would be put in UMF_1.02 { .... } scope) about 3 - dlopen will work as checks only name of the symbol. About the backward compatibility - this also needs to be discussed, because there are several options there (like introducing _v2, _v3 etc versions of functions similar to NV and using #define in headers or explicitly having _vX versions in API, etc). This could be discussed later. my general idea here is to break the compatibility NOW and try the API as much as possible to be compatible in 0.11+ |
@bratpiorka Do we actually need to 'fix' the symbols' version? The setParams* APIs and other were done so that new library version is backward compatible and we haven't changed any function declaration (only added new ones) if I'm not mistaken. If we change the symbols' version to 0.10 we will artificially break the compatibility. [2] I don't think we need to bump the symbol version on every release. We should be able to just add new symbols to the 1.0 scope and it should work fine. If we change any symbols (function params/return type) then we would need to put that symbol into a new scope or bump the scope version for all symbols. |
@igchor Ok so suppose we add a new symbol to 0.12 - how should we name the new scope? The obvious name would be UMF_0.12 but then we have "older" UMF_1.0 and "never" UMF_0.12 which could lead to some problems. Same with removing symbols Note that with #953 we already know that we would have a memory provider API mismatch between 0.10 and future 0.11+. PR #953 changes umf_memory_provider_ops_t definition that is used by umfMemoryProviderCreate() and all umfXXXMemoryProviderOps(). If we decide to merge this PR #974 we should add the new PR with 0.11 set for Memory Provider API functions, right? |
@bratpiorka did I get correctly that, according to your proposal, only a new API will have the latest version? For example, we have
I agree with @igchor that we do not want to increment the API version of a particular function every release, but just increment it when the function is changed. But this is what you proposed as well (if I get you correctly). Anyway let's have a discussion about that at UMF meeting today. |
Regarding this example, I understand that we changed the |
True, but what to do in the case when umfMemoryProviderCreate expects ops ver 0.10 and it gets ops ver 0.11? We have an assert now but we could either: ignore the extra param (remove the assert and try to handle old ops), assert in runtime (what we have for today) or assert at load time (move umfMemoryProviderCreate to a newer set). |
yes
I think the function ABI version needn't match the UMF name. These symbol versions are internals that mean "when given export was introduced" so e.g. if a symbol is in 0.10 set and now we have 1.0 this means "symbol was introduced in 0.10 and it is still valid" (using the set inheritance)
I would prefer to set all symbols to 0.10 now and do not break compatibility in 1.0
But if we keep all symbols in 1.0 version now we will be locked - what if we want to change the symbol?
Yes, I agree with this. But I propose to set all to a 0.10 version now which will be our "baseline". |
👍 Ideally we would never change existing functions though :) I have no strong opinions on changing all symbols to 0.10 now. But it might be a good idea to sort this all out right now. |
Agree with you.
Looks like we are aligned. Let's just confirm our plan at the meeting today and make a final decision. |
aca9a1e
to
aebd5ca
Compare
heh, back to my original question - I just took a peek into our release steps and there's info:
We should update it now, I guess; at least add info about minor version with breaking changes... which should only happen before version 1.0, right? 😉 Also, what about |
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.
What about proxy_lib.map
and proxy_lib.def
?
set symbol versions in def/map files