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.

Sign up for GitHub

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

Jump to bottom

[thrift] Update thrift 0.20.0 git reference #39787

Merged
merged 2 commits into from
Jul 10, 2024
Merged

[thrift] Update thrift 0.20.0 git reference #39787

merged 2 commits into from
Jul 10, 2024

Conversation

harris-josh
Copy link
Contributor

@harris-josh harris-josh commented Jul 8, 2024 •
edited
Loading

This is a candidate fix for the Thrift build failure identified in #39786.

The PR updates the port to reference the v0.20.0 tag instead of the 0.20.0 branch.

Fixes #39786.

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@WangWeiLin-MV WangWeiLin-MV added the category:port-bug The issue is with a library, which is something the port should already support label Jul 9, 2024
@autoantwort
Copy link
Contributor

I think we should not use a branch as reference at all because per definition it is not stable... Use a git tag or commit sha.

@timkpaine timkpaine mentioned this pull request Jul 9, 2024
Merged
1 task
harris-josh
harris-josh commented Jul 9, 2024
View reviewed changes
ports/thrift/portfile.cmake
@@ -12,7 +12,7 @@ vcpkg_find_acquire_program(BISON)
vcpkg_from_github(
OUT_SOURCE_PATH SOURCE_PATH
REPO apache/thrift
REF "${VERSION}"
REF "v${VERSION}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the Thrift repository is using a convention where the latest releases are all tagged vX.Y.Z, so I think this minor change should work.

I suppose this could change in the future, but I think this is probably the cleanest solution for now.

Copy link

Choose a reason for hiding this comment

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

Thrift repository is using a convention where the latest releases are all tagged vX.Y.Z

I can confirm that. I would also recommend to reference the release tag as it never changes after the release. Sorry for any confusion, was not aware that a single commit may cause such a tail of problems.

@harris-josh
Copy link
Contributor Author

@microsoft-github-policy-service agree

@harris-josh
Copy link
Contributor Author

I think we should not use a branch as reference at all because per definition it is not stable... Use a git tag or commit sha.

Makes sense to me. I updated the PR to use the corresponding tag.

@harris-josh harris-josh changed the title [thrift] Update thrift hash [thrift] Update thrift 0.20.0 git reference Jul 9, 2024
@harris-josh harris-josh marked this pull request as ready for review July 9, 2024 14:00
@WangWeiLin-MV WangWeiLin-MV added the info:reviewed Pull Request changes follow basic guidelines label Jul 10, 2024
raulcd
raulcd approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@raulcd raulcd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is holding our Apache Arrow release, could this be merged?

@raulcd raulcd mentioned this pull request Jul 10, 2024
Closed
@JavierMatosD JavierMatosD merged commit 1b5f734 into microsoft:master Jul 10, 2024
17 checks passed
@raulcd
Copy link
Contributor

raulcd commented Jul 10, 2024

Thanks @JavierMatosD !

@raulcd raulcd mentioned this pull request Jul 10, 2024
Merged
@harris-josh harris-josh deleted the update-thrift-hash branch July 10, 2024 16:19
kou pushed a commit to apache/arrow that referenced this pull request Jul 10, 2024
@raulcd
GH-43204: [CI][Packaging] Apply vcpkg patch to fix Thrift version (#4…
2ae192b 
…3208)

### Rationale for this change

Currently our java-jars and some wheels jobs are failing due to downloading a wrong version of Apache Thrift based on the 0.20.0 branch instead of the tag. That branch contains a new commit that makes the sha validation to fail.

### What changes are included in this PR?

Apply the Thrift patch that was applied on vcpkg here: microsoft/vcpkg#39787

### Are these changes tested?
Via archery

### Are there any user-facing changes?
No
* GitHub Issue: #43204

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
raulcd added a commit to apache/arrow that referenced this pull request Jul 11, 2024
@raulcd
GH-43204: [CI][Packaging] Apply vcpkg patch to fix Thrift version (#4…
bf75923 
…3208)

### Rationale for this change

Currently our java-jars and some wheels jobs are failing due to downloading a wrong version of Apache Thrift based on the 0.20.0 branch instead of the tag. That branch contains a new commit that makes the sha validation to fail.

### What changes are included in this PR?

Apply the Thrift patch that was applied on vcpkg here: microsoft/vcpkg#39787

### Are these changes tested?
Via archery

### Are there any user-facing changes?
No
* GitHub Issue: #43204

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
cenit pushed a commit to cenit/vcpkg that referenced this pull request Jul 12, 2024
@harris-josh @cenit
[thrift] Update thrift 0.20.0 git reference (microsoft#39787)
b8e10e4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jens-G Jens-G Jens-G left review comments

@raulcd raulcd raulcd approved these changes

@WangWeiLin-MV WangWeiLin-MV WangWeiLin-MV approved these changes

Assignees

@WangWeiLin-MV WangWeiLin-MV

Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[thrift] build failure
6 participants
@harris-josh @autoantwort @raulcd @Jens-G @WangWeiLin-MV @JavierMatosD