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

GH-38201: [CI][Packaging] Pin zlib 1.2.13 when using thrift on conan #38202

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

raulcd
Copy link
Member

@raulcd raulcd commented Oct 11, 2023

Rationale for this change

There is a conflict between the required Zlib version when using both thrift and GRPC.

What changes are included in this PR?

Pinning zlib when using thrifht.

Are these changes tested?

Via archery

Are there any user-facing changes?

No

@raulcd
Copy link
Member Author

raulcd commented Oct 11, 2023

@github-actions crossbow submit conan-maximum

@github-actions
Copy link

⚠️ GitHub issue #38201 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Oct 11, 2023
@github-actions
Copy link

Revision: a174afc

Submitted crossbow builds: ursacomputing/crossbow @ actions-dbb832daad

Task Status
conan-maximum Github Actions

@raulcd raulcd marked this pull request as ready for review October 11, 2023 12:21
Copy link
Member

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

Sounds logical, thanks!

@assignUser assignUser merged commit 757cf3e into apache:main Oct 11, 2023
13 checks passed
@assignUser assignUser removed the awaiting committer review Awaiting committer review label Oct 11, 2023
raulcd added a commit that referenced this pull request Oct 11, 2023
…38202)

### Rationale for this change
There is a conflict between the required Zlib version when using both thrift and GRPC.

### What changes are included in this PR?

Pinning zlib when using thrifht.

### Are these changes tested?

Via archery

### Are there any user-facing changes?

No
* Closes: #38201

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
@kou
Copy link
Member

kou commented Oct 11, 2023

In general, we should not add our workaround to only our copy as much as possible. Instead, we should fix a problem in upstream and our side as much as possible.
It's for maintainability and Conan related tests' propose. We want to avoid problems as much as possible when we update https://github.com/conan-io/conan-center-index/tree/master/recipes/arrow as a post release task. If we have changes that exist only in our side, we may not be able to detect problems in our Conan related CI jobs.

In this case, it seems that there is a fix in upstream: conan-io/conan-center-index#19803
How about synchronizing ci/conan/ with https://github.com/conan-io/conan-center-index/tree/master/recipes/arrow instead of this change?
We can use ci/conan/merge_upstream.sh for it. I can do it. But if anyone wants to do it, I can help the person.

@raulcd
Copy link
Member Author

raulcd commented Oct 12, 2023

Thanks @kou ! Sorry, I should have already done that. I've gave it a try on this PR: #38237

llama90 pushed a commit to llama90/arrow that referenced this pull request Oct 12, 2023
…conan (apache#38202)

### Rationale for this change
There is a conflict between the required Zlib version when using both thrift and GRPC.

### What changes are included in this PR?

Pinning zlib when using thrifht.

### Are these changes tested?

Via archery

### Are there any user-facing changes?

No
* Closes: apache#38201

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 757cf3e.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
…conan (apache#38202)

### Rationale for this change
There is a conflict between the required Zlib version when using both thrift and GRPC.

### What changes are included in this PR?

Pinning zlib when using thrifht.

### Are these changes tested?

Via archery

### Are there any user-facing changes?

No
* Closes: apache#38201

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…conan (apache#38202)

### Rationale for this change
There is a conflict between the required Zlib version when using both thrift and GRPC.

### What changes are included in this PR?

Pinning zlib when using thrifht.

### Are these changes tested?

Via archery

### Are there any user-facing changes?

No
* Closes: apache#38201

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…conan (apache#38202)

### Rationale for this change
There is a conflict between the required Zlib version when using both thrift and GRPC.

### What changes are included in this PR?

Pinning zlib when using thrifht.

### Are these changes tested?

Via archery

### Are there any user-facing changes?

No
* Closes: apache#38201

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
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 this pull request may close these issues.

[CI][Packaging] Conan maximum fails with zlib conflict
3 participants