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

[azure-core-tracing-opentelemetry-cpp | azure-core-cpp] Add new port and publish azure-core-cpp version to 1.0.0-beta.2 #25507

Conversation

azure-sdk
Copy link
Contributor

Update vcpkg ports for Azure SDK release. This release may contain multiple ports.

## 1.0.0-beta.2 (2022-06-30)

### Breaking Changes

- The `Azure::Core::Tracing::OpenTelemetry::OpenTelemetryProvider` type can only be instantiated via a factory method: `OpenTelemetryProvider::Create()`.

### Other Changes

- Removed `_internal` APIs from the public API surface. Also removed most of the `_internal` APIs from the public `opentelemetry.hpp` headers.
@azure-sdk
Copy link
Contributor Author

Adding azure-core-tracing-opentelemetry to release

cc: @RickWinter, @ahsonkhan, @antkmsft, @vhvb1989, @gearama, @LarryOsterman

github-actions[bot]
github-actions bot previously approved these changes Jun 30, 2022
## 1.7.0 (2022-06-30)

### Features Added

- Added prototypes and initial service support for Distributed Tracing.
@azure-sdk
Copy link
Contributor Author

Adding azure-core to release

cc: @RickWinter, @ahsonkhan, @antkmsft, @vhvb1989, @gearama, @LarryOsterman

@JonLiu1993 JonLiu1993 self-assigned this Jul 1, 2022
@JonLiu1993 JonLiu1993 added category:port-update The issue is with a library, which is requesting update new revision category:new-port The issue is requesting a new library to be added; consider making a PR! labels Jul 1, 2022
@JonLiu1993 JonLiu1993 changed the title [azure-core-tracing-opentelemetry-cpp] publish version 1.0.0-beta.2 [azure-core-tracing-opentelemetry-cpp | azure-core-cpp] Add new port and publish azure-core-cpp version to 1.0.0-beta.2 Jul 1, 2022
@JonLiu1993
Copy link
Member

All features are tested successfully in the following triplet:

  • x86-windows
  • x64-widnows
  • x64-windows-static

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Jul 1, 2022
@@ -10,8 +10,7 @@
"dependencies": [
{
"name": "openssl",
"platform": "!windows & !uwp",
"version>=": "1.1.1n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This isn't the only Azure port using version>=

Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the platform restriction - is openssl used on Windows?
Why remove the version restriction?

Copy link
Member

Choose a reason for hiding this comment

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

  1. OpenSSL is used on both Windows and Linux.
  2. "version>=" requires semantic versioning. The text "1.1.1n" is not a valid semantic version.

It's a bug in the vcpkg tooling that this is allowed.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should work for all schemes. But version-string is different indeed, >= only applies to port-version, 1.1.1n would need to match exactly. (Maybe it should just ask for 3.0.0.)

Copy link
Member

Choose a reason for hiding this comment

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

One alternative to updating the version dependency is to simply remove the version dependency from the package definition.

Why do we need to have a specific package version?

@BillyONeal Do you know of a reason we should add back our package version dependency for azure-core?

Copy link
Member

Choose a reason for hiding this comment

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

Umm. We are currently using OpenSSL 3.0.x, so the bespoke version number is irrelevant. My change above removes the dependency on the bespoke version number.

Given that we're no longer using the bespoke version number, why do I need to explicitly provide a version number for OpenSSL?

And if we're required to provide a version number for OpenSSL, why aren't we required to provide a version number for the other packages we depend on?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it is not needed anymore. I could see an argument for changing it to >=3.0.0 to disallow old 1.1.1* versions if you want that.

Copy link
Member

@antkmsft antkmsft Jul 14, 2022

Choose a reason for hiding this comment

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

@LarryOsterman, should we put >=3.0.0 here? Let's get this PR merged, so that #25629 can be merged after that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's worth re-releasing the package to add it. As the package is currently written, it is correct.

In the future we might want to consider adding the OpenSSL dependency.

Copy link
Member

@ahsonkhan ahsonkhan Jul 14, 2022

Choose a reason for hiding this comment

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

@BillyONeal, @vicroms is this PR ready to merge, as-is, or is there a correctness change we need to make?

{
"name": "azure-core-cpp",
"default-features": false,
"version>=": "1.7.0-beta.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't have any effect

Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't it have any effect?

How do I author a package version restriction to reflect that a package depends on another package version?

Copy link
Member

Choose a reason for hiding this comment

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

I believe that this will have an effect, if for example someone's baseline doesn't include this port version and the user tries to pick a newer one with a constraint themselves. But yes, most customers will be updating to a baseline where this constraint is meaningless.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, Billy's logic is explicitly why the version requirement is there. We need this to fail if the customer's baseline does not include 1.7.0-beta.1.

@dan-shaw dan-shaw added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed info:reviewed Pull Request changes follow basic guidelines labels Jul 2, 2022
@vicroms vicroms added requires:author-response and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Jul 12, 2022
@vicroms vicroms merged commit fd117d2 into microsoft:master Jul 14, 2022
@ahsonkhan ahsonkhan deleted the azure-sdk-for-cpp-azure-core-tracing-opentelemetry-1678545 branch July 14, 2022 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! category:port-update The issue is with a library, which is requesting update new revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants