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

Test build of 34c0dd5 #90

Merged
merged 51 commits into from Oct 11, 2022
Merged

Test build of 34c0dd5 #90

merged 51 commits into from Oct 11, 2022

Conversation

sgillies
Copy link
Member

That's the no _loading rasterio PR rasterio/rasterio#2594.

@snowman2
Copy link
Member

Added a fix related to pyproj4/pyproj#1132

@sgillies
Copy link
Member Author

@snowman2 not all heroes wear capes 🦸

id: vcpkgcache
with:
path: C:/vcpkg/installed
key: ${{ runner.os }}-vcpkg-${{ hashFiles('.github/workflows/*') }}
key: ${{ runner.os }}-dependencies-${{ hashFiles('.github/workflows/*', 'vcpkg.json') }}
Copy link
Member Author

Choose a reason for hiding this comment

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

@snowman2 while you're saving my bacon, do you see anything obviously wrong with the above? I'm having the experience of not seeing the cache miss when I change vcpkg.json.

Copy link
Member

Choose a reason for hiding this comment

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

At a glance, it looks fine. I guess we'll just have to see if it works 🤞.

In case it is helpful, pyproj uses the expected PROJ version in the key to update the cache: https://github.com/pyproj4/pyproj/blob/bc7805ccc813b40ab28099ef305821300f149aad/.github/workflows/release.yaml#L93.

@snowman2
Copy link
Member

Glad to see the fix worked 🎉


- name: Install GDAL
run: |
vcpkg install gdal:x64-windows
vcpkg install --feature-flags="versions" gdal:x64-windows
Copy link

Choose a reason for hiding this comment

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

So vcpk install gdal is also what I did originally in the pyogrio wheel builds, but when we moved to use a manifest file (to pin the version, or provide overrides), some more things changed:

  • gdal is now listed as package to install in the manifest, so you don't have to (or can't?) mention it here, I think
  • We are also using --feature-flags="manifests" in addition to "versions", although it is not clear from the documentation whether this is needed (if there is a manifest file in the CWD, that might be sufficient. EDIT: based on the comment in --feature-flags command line arg is not documented microsoft/vcpkg#24474, that might indeed be sufficient to be "in manifest mode")
  • We also pointed to the directory where the manifest file is living with --x-manifest-root=./ci (although this probably not needed here since here it lives top-level)

See the full diff of geopandas/pyogrio#69

Copy link
Member Author

Choose a reason for hiding this comment

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

@jorisvandenbossche thank you! I'm going to try removing gdal:x64-windows from the command line. Maybe it's negating the manifest? Hard to say from the vcpkg docs.

vcpkg.json Outdated
"overrides": [
{ "name": "gdal", "version": "3.4.3#2" }
{ "name": "proj:x64-windows", "version": "9.0.1" }

Choose a reason for hiding this comment

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

You can specify the default triplet as an Env variable (see eg https://github.com/geopandas/pyogrio/blob/d1714041153416c0358658746a6b95f067c835a5/.github/workflows/release.yml#L165), so you don't need to specify it here (that causes the current error I think)

Choose a reason for hiding this comment

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

And same for gdal above

Choose a reason for hiding this comment

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

(it's also the default triplet, I assume, so your last commit to just remove it might indeed be sufficient)

Copy link
Member Author

Choose a reason for hiding this comment

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

So far so good! 🤞

@snowman2
Copy link
Member

snowman2 commented Oct 5, 2022

I just added: --x-install-root=$VCPKG_INSTALLATION_ROOT/installed

From: https://github.com/pyproj4/pyproj/blob/5723f370287bc71b9d6de80b8acf3ea517931ed2/.github/workflows/release.yaml#L104

@snowman2
Copy link
Member

snowman2 commented Oct 5, 2022

looks like you already added it and removed it

@sgillies
Copy link
Member Author

sgillies commented Oct 5, 2022

@snowman2 yeah, I was using that (following your example) in 1231248, but vcpkg seemed to disregard it and and installed packages to locations like c:/vcpkg/packages/gdal_x64-windows (see https://github.com/rasterio/rasterio-wheels/actions/runs/3192686214/jobs/5210441743#step:5:1014), which is not helpful at all. vcpkg is slowing driving me insane.

@snowman2
Copy link
Member

snowman2 commented Oct 6, 2022

It would be really nice if there was a temporary machine to log into and explore after the vcpkg install completed. Would really make this process a lot less painful.

@rbuffat
Copy link

rbuffat commented Oct 6, 2022

Not sure if it works well on windows, but with this action it is possible to connect to a workflow session: https://github.com/mxschmitt/action-tmate

@snowman2
Copy link
Member

snowman2 commented Oct 6, 2022

Not sure if it works well on windows, but with this action it is possible to connect to a workflow session: https://github.com/mxschmitt/action-tmate

That looks really useful.

shell: bash
run: |
echo -e "\n[build_ext]" >> setup.cfg
echo "include_dirs = C:/vcpkg/installed/x64-windows/include" >> setup.cfg
Copy link
Member

Choose a reason for hiding this comment

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

I noticed this used to be capitalized here and it wasn't below. I am wondering if it is case sensitive.

@sgillies
Copy link
Member Author

sgillies commented Oct 7, 2022

@snowman2 there it is. vcpkg switched on me, it's installing stuff to installed/x86-windows now instead of installed/x64-windows: https://github.com/rasterio/rasterio-wheels/actions/runs/3205013336/jobs/5236997241#step:9:9. What a terrible system. I suspect explicitly specifying the triplet will solve this.

@snowman2
Copy link
Member

snowman2 commented Oct 7, 2022

Good eye 👍

I suspect explicitly specifying the triplet will solve this.

https://github.com/pyproj4/pyproj/blob/3009c5c9ab8cbffc30a39831bc9624906f9e388e/.github/workflows/release.yaml#L97-L98

        env:
          VCPKG_DEFAULT_TRIPLET: ${{ matrix.triplet }}

@snowman2
Copy link
Member

snowman2 commented Oct 7, 2022

Progress! Only 1 failure:


data = local('C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\test_copyfiles_same_dataset_an0')

    def test_copyfiles_same_dataset_another_name(data):
        """A dataset can't be copied to itself by another name."""
        path = str(data.join('RGB.byte.tif'))
        with pytest.raises(RasterioIOError):
>           rasterio.shutil.copyfiles(path, 'file://' + path)
E           Failed: DID NOT RAISE <class 'rasterio.errors.RasterioIOError'>

@sgillies
Copy link
Member Author

sgillies commented Oct 7, 2022

Thanks for the assist @snowman2 ! I guess something changed in 3.5.2? We're not current in rasterio's tests: https://github.com/rasterio/rasterio/actions/runs/3206825922.

@snowman2
Copy link
Member

snowman2 commented Oct 7, 2022

Thanks for working on this @sgillies 👍 - this one was definitely interesting to figure out.

@snowman2
Copy link
Member

snowman2 commented Oct 7, 2022

We're not current in rasterio's test

rasterio/rasterio#2606

@sgillies
Copy link
Member Author

@snowman2 @vincentsarago https://github.com/rasterio/rasterio-wheels/actions/runs/3206563664 has two failures on Windows despite all the tests passing with the ubuntu-small-3.5.2 image. I'm inclined to skip these for the 1.3.3 release and go ahead with a little bit of undefined behavior, hopefully resolving the issues before 1.3.4.

@sgillies sgillies marked this pull request as ready for review October 11, 2022 20:55
@sgillies sgillies merged commit 0eed1dd into main Oct 11, 2022
@sgillies sgillies deleted the sgillies-patch-1 branch January 25, 2023 18:47
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.

None yet

4 participants