Adds opentdf-client versions 1.4.0 and 1.5.0 #18896
Conversation
|
|
|
I detected other pull requests that are modifying opentdf-client/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
| if Version(self.version) >= "1.5.0": | ||
| self.requires("openssl/3.1.1") | ||
| else: | ||
| self.requires("openssl/1.1.1q") |
There was a problem hiding this comment.
Hi @patmantru, thanks a lot for the contribution. One comment: we are now accepting version ranges for openssl to avoid overrides: https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/dependencies.md#version-ranges
So maybe you can use one for both those requires.
There was a problem hiding this comment.
Thanks for the suggestion, but it's not going to work in this case. Our code requires the OpenSSL 1.x interface for 1.4.0-and-older versions. The changes to support OpenSSL 3.x were introduced in our version 1.5.0. I need to leave that check as-is.
There was a problem hiding this comment.
Hi @patmantru, I may have not explained it properly, I mean that you can use separate version ranges for each Version(self.version) so that you avoid the need of doing overrides in other libraries that may be required in the same graph. Something similar to this:
if Version(self.version) >= "1.5.0":
self.requires("openssl/[>=3.1 <4]")
else:
self.requires("openssl/[>=1.1 <3]")
There was a problem hiding this comment.
OK, I understand now. The issue I have (especially for the 1.1.x stuff) is that I don't want someone to accidentally pull in a random version that we don't know anything about. Case in point, there are vulnerabilities in the 1.1.1[a-z] earlier versions that are fixed in later ones, so I'd rather go with a fixed version. Unless there's a strict CCI requirement that I change this, I'd really prefer to leave it as-is. Having a repeatable build (identical binaries every time you build it) is much more important to me for this package.
There was a problem hiding this comment.
@czoido any additional feedback?
There was a problem hiding this comment.
I have been building this using a local recipe, and it works fine using pinned versions. Now that I am specifying a version range...the opentdf-client version 1.5.0 that includes the OpenSSL 3.1.1 upgrade changes fails because jwt-cpp disallows it. Going back to pinned versions to see if that works.
There was a problem hiding this comment.
@jcar87 After 12 (!!!) attempts and the better part of 2 full work days, I have this PR building.
Notes:
jwt-cpp has its requirement for OpenSSL set as the range [>1.1.1c, <1.1.1u] (note the less-than on the upper limit) which means any range spec that pulls in something newer than 1.1.1t will fail. The opentdf-client package uses OpenSSL 1.1.1 for releases up through 1.4.0. Since the current 1.x version is 1.1.1u, setting a range spec of [>1.1.1q <1.2] will pick 1.1.1u, which will fail jwt-cpp's range check. As a concession to your request, even though it is actually propagating the problem, I've added [>1.1.1q <1.1.1u] as the range for opentdf-client 1.4.0 and older, just so I can say I did my best here.
Similarly, specifying an OpenSSL 3.1.x version range for my release 1.5.0 recipe will fail, because it's ALSO not within jwt-cpp's [>1.1.1c, <1.1.1u] range check.
The ONLY WAY TO GET MY RELEASE 1.5.0 RECIPE FOR TO BUILD CLEANLY given the jwt-cpp recipe, is to pin the OpenSSL version as 3.1.1, which overrides the jwt-cpp range check. This is evident from looking at the 11 previous build attempts that all failed for one range-check reason or another.
I think I've done more than my share of attempting to comply with your request to use version ranges for OpenSSL. I've proven that it is not possible in this situation given all the constraints. All I wanted to do was to publish our release 1.4.0 and 1.5.0 recipe updates. That was supposed to be a couple of new SHA256 lines, and bumping some requirement versions. Now this has dragged on for weeks, and it's still not in.
Please approve this so it can be merged and I can move on to more important things...like upgrading the recipe to be conan 2.x compliant.
Thanks-
Pat
There was a problem hiding this comment.
As someone developing this code for a data protection company, security is job 1. That includes build repeatability. That means knowing exactly what is being used. That means version pinning. Everything.
Absolutely agreed, this is important, and possible with Conan Center and where we are headed. I think we have a disagreement as to where this version pinning should be happening. We want to make sure that it is consumers that are able to do version pinning. This is the purpose of lockfiles. This is a good summary of recommendations, that places the responsibility on consumers to be aware and known their entire dependency tree - we give them the options to both pin the version and update to a newer one if necessary, provided the newer one is known to be compatible (as is the case with the OpenSSL 3.x series).
That makes me sad, as I had hoped CCI would be like (as you mentioned) Pypi/Cargo/npm, a central 'this is the gold standard for package recipes' for C++ modules.
on the one hand, you have hopes that CCI would be like Pypi/Cargo/npm, but on the other hand these tools and repositories use version ranges by default, which you are arguing against - Some example
- These are the requirements of
boto3, the top most downloaded package in PyPI - notice how each of them uses a version range, as we are advocating to use in Conan Center for OpenSSL. - Cargo for Rust uses version ranges by default, even when a full version is specified - this is because it assumes SemVer compatibility, same as is the case for OpenSSL.
- The semver npm package (which has 276 million weekly downloads) lists has a dependency that uses version ranges: https://github.com/npm/node-semver/blob/main/package.json#L51 - the
^notation here is also exactly equivalent as what we are doing for OpenSSL.
In all these cases, it means that users doing pip install, npm install or cargo builds would get a combination of versions today, and will very likely get a completely different one if they run the same commands 2 weeks from now. In all of these cases there are well described mechanisms to lock the version resolver to specific versions:
- pip: https://pip.pypa.io/en/stable/user_guide/#constraints-files
- npm: https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json
- cargo: https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html
The practices that we are proposing for Conan Center are in line with widely adopted practices elsewhere. Conan is no differente: https://docs.conan.io/2.0/tutorial/versioning/lockfiles.html
The CCI focus now seems to be on doing whatever is necessary to reduce the volume of recipe updates
This is categorically untrue.
including floating versions 'on some packages' to force more of the builds to work without intervention. 'Flexibility' is not always better.
The "some packages" is for packages that have known compatibility promises. For libraries that don't, we won't be using them. Widely adopted practice elsewhere.
then can I upload them to CCI with the recipe and ensure they are used when the recipe is referenced by a consumer? Guessing not, because that would defeat CCI's 'flexibility' goal.
While there are some differences, Cargo explains this better than I can: https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries
We want the end consumers to be in control - they depend on OpenSSL, they should control which version, provided all the libraries they are using are compatible.
As I mentioned in another issue (#13994), it seems like the CCI guidance continues to be 'Please submit your recipes to CCI, but you should really make a local copy of everything and use that instead of CCI because we're unreliable', which falls far short of what CCI could be.
The PR is still open and subject to change. The advise is, and always has been for Conan Center, to avoid consuming recipes and binaries from Conan Center in a fully unconstrained manner. The advice (especially with Conan 2.0) would be to use lockfiles, and only update the lockfiles when you need to update your dependencies - then you can inspect and solve conflicts if needed or fix compatibility issues. It shouldn't happen "from under you" - this is in line with the recommendations for python packages, npm, cargo, and so on.
There was a problem hiding this comment.
Notably, the recipe for jwt-cpp specs "openssl/[>1.1.1c, <1.1.1u]", which means my specification of [>=1.1 <=1.2] fails because 1.1.1u (most recent openSSL version) is not acceptable to jwt-cpp. Rather than picking 1.1.1t, which would be acceptable to both my package and jwt-cpp, it just fails. This is like playing whack-a-mole, it moves the versioning problem instead of fixing it.
Thanks for reporting this. openssl/[>1.1.1c, <1.1.1u] is not accepted range in Conan Center. We have discovered there is a bug with the bump dependencies automerge - and the team is working to fix this. I have opened a PR to address this: #19220
I apologise you experienced this issues, I can understand this has been frustating.
The ONLY WAY TO GET MY RELEASE 1.5.0 RECIPE FOR TO BUILD CLEANLY given the jwt-cpp recipe, is to pin the OpenSSL version as 3.1.1, which overrides the jwt-cpp range check.
Unfortunately this is a side effect of the openssl/[>1.1.1c, <1.1.1u] dependency in the jwt-cpp recipe - this is malformed and should have not been merged. The team is looking into this to avoid this in the future. in Conan 2.0 this is interpreted as openssl/[>1.1.1c], hence why it works when you pin to 3.1.1: in Conan 1 it does an override, and in Conan 2.0 it just matches the range.
Please note that this silent overrides (as experienced in Conan 1.x) are no longer a thing in Conan 2.0: if jwt-cpp were only compatible with OpenSSL 1.x and opentdf-client is only compatible with OpenSSL 3.x onwards (or in the case where both depend on a specific version), I'm fairly certain this would result in a conflict immediately, and there is no override. Version ranges avoid the conflicts when things are known to be compatible.
There was a problem hiding this comment.
We are fixing this issue here: #19220 and will make sure opentdf-client is relaunched and merged today!
|
Anything else we're waiting for in order to get the needed approvals? @czoido |
|
@czoido any way to get some more reviews/approvals on this? It's a very small change (adds 2 versions, adds 1 dependency, upgrades another dependency) so it should be a quick review. Is there anything besides 2 more reviews that this needs to get merged? Thanks... |
|
@czoido still waiting for additional reviews, and the CI doesn't look like it's run yet. Can you provide any guidance on how long I should expect this to take to get merged? |
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit abbaa72opentdf-client/1.2.0opentdf-client/1.3.2opentdf-client/1.3.10opentdf-client/1.4.0 |
…onan-center-index into opentdf-client-version-1.5.0
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…version to override
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit ebb08c7opentdf-client/1.3.10opentdf-client/1.5.0opentdf-client/1.4.0opentdf-client/1.1.2opentdf-client/1.3.2opentdf-client/1.3.8opentdf-client/1.1.3opentdf-client/1.1.5opentdf-client/1.3.6opentdf-client/1.3.3opentdf-client/1.3.9opentdf-client/1.3.4opentdf-client/1.2.0opentdf-client/1.1.6 |
Hi @patmantru, sorry for not responding I was out of the office for some days. I'll have a look again now to this PR. Thanks for your patience. |
Conan v1 pipeline ✔️All green in build 19 (
Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this will be required for pull requests to be merged in the near future. See details:Failure in build 19 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Hooks produced the following warnings for commit 61d8cf7opentdf-client/1.1.2opentdf-client/1.1.3opentdf-client/1.3.10opentdf-client/1.3.9opentdf-client/1.1.6opentdf-client/1.3.3opentdf-client/1.3.8opentdf-client/1.3.6opentdf-client/1.4.0opentdf-client/1.5.0opentdf-client/1.2.0opentdf-client/1.3.2opentdf-client/1.3.4opentdf-client/1.1.5 |
* Adds versions 1.4.0 and 1.5.0 * Need magic_enum for 1.4.0 also * Update to new release content * Update to newest release tag content * Change OpenSSL version references to ranges per CCI requirements * fix typo * Try different range spec for 1.1.x * Try another range spec for openssl 1.1.x * Another try at version ranges * Still another try at ranges for openssl 1.1.x * Pick a version range that is also compatible with jwt-cpp * openssl/[>=3.1 <3.2] range is not compatible with jwt-cpp. Use fixed version to override * One more try with newer jwt-cpp and ranges * Avoid jwt-cpp complaint about OpenSSL 1.1.1u * Override 3.1.1 for openssl to avoid jwt-cpp complaint * jwt-cpp requires are broken forr 0.5.0 and 0.6.0, avoid * opentdf-client: update openssl version ranges --------- Co-authored-by: Rubén Rincón Blanco <rubenrb@jfrog.com> Co-authored-by: Luis Caro Campos <3535649+jcar87@users.noreply.github.com>
Specify library name and version: opentdf-client/1.5.0