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

[highfive] fix SHA of downloading patch #29708

Merged
merged 11 commits into from
Feb 27, 2023

Conversation

gouarin
Copy link
Contributor

@gouarin gouarin commented Feb 17, 2023

fix #29690

@gouarin
Copy link
Contributor Author

gouarin commented Feb 17, 2023

@microsoft-github-policy-service agree

github-actions[bot]
github-actions bot previously approved these changes Feb 17, 2023
@LilyWangLL LilyWangLL added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. and removed info:internal This PR or Issue was filed by the vcpkg team. labels Feb 17, 2023
@LilyWangLL LilyWangLL changed the title [highfive] fix SHA [highfive] fix SHA of downloading patch Feb 17, 2023
github-actions[bot]
github-actions bot previously approved these changes Feb 17, 2023
@LilyWangLL
Copy link
Contributor

Please also modify the port-version in ports/highfive/vcpkg.json, you can refer to doc https://github.com/microsoft/vcpkg-docs/blob/main/vcpkg/contributing/maintainer-guide.md#versioning.
Then you need run command './vcpkg x-add-version highfive --overwrite-version' after commit new change.

LilyWangLL
LilyWangLL previously approved these changes Feb 17, 2023
@LilyWangLL LilyWangLL added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Feb 17, 2023
github-actions[bot]
github-actions bot previously approved these changes Feb 17, 2023
@gouarin
Copy link
Contributor Author

gouarin commented Feb 21, 2023

What is the delay to merge these kinds of PR ?

@dg0yt
Copy link
Contributor

dg0yt commented Feb 21, 2023

What is the delay to merge these kinds of PR ?

There is a group of people which do review. There is another group of people which do merges. There are also community members.

Comment on lines 1 to 67
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 7f8f8135d..11627bb7e 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -28,26 +28,28 @@ jobs:
# Job testing compiling on several Ubuntu systems + MPI
# =========================================================
#
- # For 18.04: bare HighFive
- # For 20.04: activate Boost, OpenCV
+ # For 20.04: bare and activate Boost, OpenCV
# For latest: activate Boost, Eigen, OpenCV, with Ninja
#
# XTensor tests are run for conda/mamba and MacOS
Linux_MPI:
- runs-on: ${{matrix.os}}
+ runs-on: ${{matrix.config.os}}
+ name: ${{toJson(matrix.config)}}
strategy:
matrix:
- os: [ubuntu-18.04, ubuntu-20.04, ubuntu-latest]
include:
- - os: ubuntu-18.04
- pkgs: ''
- flags: '-DHIGHFIVE_USE_BOOST:Bool=OFF'
- - os: ubuntu-20.04
- pkgs: 'libboost-all-dev libopencv-dev'
- flags: '-DHIGHFIVE_USE_OPENCV:Bool=ON -GNinja'
- - os: ubuntu-latest
- pkgs: 'libboost-all-dev libeigen3-dev libopencv-dev'
- flags: '-DHIGHFIVE_USE_EIGEN:Bool=ON -DHIGHFIVE_USE_OPENCV:Bool=ON -GNinja'
+ - config:
+ os: ubuntu-20.04
+ pkgs: ''
+ flags: '-DHIGHFIVE_USE_BOOST:Bool=OFF'
+ - config:
+ os: ubuntu-20.04
+ pkgs: 'libboost-all-dev libopencv-dev'
+ flags: '-DHIGHFIVE_USE_OPENCV:Bool=ON -GNinja'
+ - config:
+ os: ubuntu-latest
+ pkgs: 'libboost-all-dev libeigen3-dev libopencv-dev'
+ flags: '-DHIGHFIVE_USE_EIGEN:Bool=ON -DHIGHFIVE_USE_OPENCV:Bool=ON -GNinja'

steps:
- uses: actions/checkout@v3
@@ -62,11 +64,11 @@ jobs:
- name: "Install libraries"
run: |
sudo apt-get -qq update
- sudo apt-get -qq install libhdf5-openmpi-dev libsz2 ninja-build ${{ matrix.pkgs }}
+ sudo apt-get -qq install libhdf5-openmpi-dev libsz2 ninja-build ${{ matrix.config.pkgs }}

- name: Build
run: |
- CMAKE_OPTIONS=(-DHIGHFIVE_PARALLEL_HDF5:BOOL=ON ${{ matrix.flags }})
+ CMAKE_OPTIONS=(-DHIGHFIVE_PARALLEL_HDF5:BOOL=ON ${{ matrix.config.flags }})
source $GITHUB_WORKSPACE/.github/build.sh

- name: Test
diff --git a/deps/catch2 b/deps/catch2
index 182c910b4..ab6c7375b 160000
--- a/deps/catch2
+++ b/deps/catch2
@@ -1 +1 @@
-Subproject commit 182c910b4b63ff587a3440e08f84f70497e49a81
+Subproject commit ab6c7375be9a8e71ee84c6f8537113f9f47daf99
Copy link
Contributor

Choose a reason for hiding this comment

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

Patches in vcpkg shall be minimal. These chunks do not belong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a core developer of highfive and I just made this PR to fix its installation using vcpkg.
It's probably time to ping a core developer o highfive and the maintainer of the vcpkg package.
Do you suggest making a new release with the patch, then?

@alkino @Neumann-A

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do I get a ping? The patch here chunks here seem unnecessary for vcpkg and should thus be removed. Alternatively download the patch and apply it instead of vendoring it in vcpkg (although that comes with its own problems)

Copy link
Contributor

Choose a reason for hiding this comment

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

FTR this PR exists because the problem already arrived.

@gouarin Let people with vcpkg repo access do the change if you are not willing to do it. They should already be subscribed, and dealing with such issues is their responsibility during review IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ping you because you are the author of the update of highfive where you added a patch from a url

vcpkg_download_distfile(CATCH2_PATCH
URLS https://patch-diff.githubusercontent.com/raw/BlueBrain/HighFive/pull/669.diff
FILENAME ${PORT}-669-145454fc.diff
SHA512 b88895daa6305a3ef164f80f996bedb64e281bde9bbab893ee9190d3012ac00ad9407e3b20613fc3464f417eb0c063f7961e383213553b639491d69e145454fc
)

This is my first time doing a PR in vcpkg and you probably give me your feedback on why you applied this patch.

@dg0yt suggested me to make a local patch. But this patch is not minimal and refers to another patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dg0yt I just wanted to help. Sorry if I don't make things as expected. And I like to understand how things work.

@JavierMatosD JavierMatosD removed the info:reviewed Pull Request changes follow basic guidelines label Feb 23, 2023
@LilyWangLL LilyWangLL added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Feb 27, 2023
@JavierMatosD JavierMatosD merged commit 65d4be0 into microsoft:master Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

[highfive] Build error
5 participants