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

Prepare release v.1.9.0 (tentative: 3rd March) #2007

Closed
lalitb opened this issue Feb 27, 2023 · 15 comments · Fixed by #2018
Closed

Prepare release v.1.9.0 (tentative: 3rd March) #2007

lalitb opened this issue Feb 27, 2023 · 15 comments · Fixed by #2018
Assignees

Comments

@lalitb
Copy link
Member

lalitb commented Feb 27, 2023

There are few Open PRs which preferably be merged (not in order of priority) before creating the release:

#2000 is ABI breaking change for the SDK which should be fine in general. But it also means that if someone is using otel-cpp-sdk*so targets with their application, the application can crash if these shared library is rebuild for new version, without recompiling/relinking the application against these new shared libraries. This needs to be mentioned in the release notes.

@marcalff
Copy link
Member

marcalff commented Mar 3, 2023

Please revert the fix done with PR: Rename the global SDK version variables to avoid naming clash #2011 .

OPENTELEMETRY_SDK_MAJOR_VERSION needs to be a pre processor symbol to fix issue Provide major/minor/patch version macros #2012 .

@marcalff
Copy link
Member

marcalff commented Mar 3, 2023

@lalitb @ThomsonTan @esigo @owent Please see above comment.

@ThomsonTan
Copy link
Contributor

Please revert the fix done with PR: Rename the global SDK version variables to avoid naming clash #2011 .

OPENTELEMETRY_SDK_MAJOR_VERSION needs to be a pre processor symbol to fix issue Provide major/minor/patch version macros #2012 .

Is the revert required? I think the PR #2011 is separate the the ask of #2012 which looks like a feature request? The PR was targeting a different issue of name clashing.

@owent
Copy link
Member

owent commented Mar 3, 2023

I found some commits are missed in #2005,Can I push them next monday?I added a CI job and a unit test to check #1603.

@ThomsonTan
Copy link
Contributor

I found some commits are missed in #2005,Can I push them next Monday added a CI job and a unit test to check #1603.

It looks good to me. We can wait it before completing the release. @lalitb @marcalff @esigo how do you think?

@marcalff
Copy link
Member

marcalff commented Mar 3, 2023

Please revert the fix done with PR: Rename the global SDK version variables to avoid naming clash #2011 .
OPENTELEMETRY_SDK_MAJOR_VERSION needs to be a pre processor symbol to fix issue Provide major/minor/patch version macros #2012 .

Is the revert required? I think the PR #2011 is separate the the ask of #2012 which looks like a feature request? The PR was targeting a different issue of name clashing.

I think the revert is required.

Code introduced:

namespace sdk
{
namespace version
{
extern const int OPENTELEMETRY_SDK_MAJOR_VERSION;

will break once pre processor symbol is added for #2012 :

#define OPENTELEMETRY_SDK_MAJOR_VERSION 1

@ThomsonTan
Copy link
Contributor

OPENTELEMETRY_SDK_MAJOR_VERSION

Ok, but since as the preprocessor symbol #define OPENTELEMETRY_SDK_MAJOR_VERSION is new, probably we can name it as OPENTELEMETRY_CPP_SDK_MAJOR_VERSION? We cannot use the global var name MAJOR_VERSION anyway as it is causing more general clashing.

@lalitb
Copy link
Member Author

lalitb commented Mar 3, 2023

The intended purpose of the existing const variables was to stamp the version information in the binary.

$strings /usr/local/lib/libopentelemetry_version.a  | grep 1.8.2
pre_release_1.8.2
1.8.2
1.8.2-NONE-NONE-37-pre_release_1.8.2-435ce60f233b6718aaa04bb7068dd641b536299b

We shouldn't be removing them, or replacing them with macros. Macros should be introduced separately for #2012. Though it should be fine to change these variable names to lowercase (or something else) in case of conflict.

@marcalff
Copy link
Member

marcalff commented Mar 3, 2023

I agree these const variables should be available in the library, this is not the problem.

The problem is that resolving one name clash for MAJOR_VERSION created another name clash instead.

Ok to change to lowercase instead:

namespace sdk
{
namespace version
{
extern const int major_version;

This avoid the conflict with MAJOR_VERSION, and with OPENTELEMETRY_SDK_MAJOR_VERSION anticipated for #2012

@marcalff
Copy link
Member

marcalff commented Mar 3, 2023

@ThomsonTan @lalitb

So, if sdk::version::major_version looks ok to everyone, no need for a full revert, but some adjustments are needed instead before publishing the release.

@ThomsonTan
Copy link
Contributor

Ok. I'll make a separate PR to change the C++ version variables into lower case.

@marcalff
Copy link
Member

marcalff commented Mar 3, 2023

@lalitb About the version name, why "1.9.2" instead of "1.9.0" ?

@lalitb lalitb changed the title Prepare release v.1.9.2 (tentative: 3rd March) Prepare release v.1.9.0 (tentative: 3rd March) Mar 3, 2023
@lalitb
Copy link
Member Author

lalitb commented Mar 3, 2023

@lalitb About the version name, why "1.9.2" instead of "1.9.0" ?

Thanks for correcting me. Updated to "1.9.0".

@owent
Copy link
Member

owent commented Mar 6, 2023

#2005 and #2000 are all finished now.

@marcalff
Copy link
Member

marcalff commented Mar 6, 2023

Can we also add this to the next release:

I will generate new semconv once the spec label is ready.

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 a pull request may close this issue.

5 participants