Conversation
|
@copilot look at the windows failure and figure out why the cram binary cannot be found on windows |
There was a problem hiding this comment.
Pull request overview
This PR adds Windows support to the CI pipeline by including windows-latest in the test matrix and adapting the workflow to handle platform-specific requirements for GDAL installation and test execution.
- Added Windows runner to the CI matrix alongside existing Ubuntu and macOS runners
- Modified GDAL installation for Windows to use a third-party GitHub Action instead of Chocolatey
- Split test execution into separate Unix and Windows steps to accommodate PowerShell syntax differences
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pyproject.toml | Added pip to build-system requirements (though this may not be necessary) |
| .github/workflows/ci.yml | Added Windows runner to matrix, reorganized setup steps, implemented Windows-specific GDAL installation and test execution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/ci.yml
Outdated
| run: choco install gdal | ||
| - name: install pip (Windows) | ||
| if: runner.os == 'Windows' | ||
| run: uv pip install pip cram |
There was a problem hiding this comment.
cram is being installed twice in the Windows workflow: once in this step (line 37) and again in the "Test (Windows)" step (line 56). The installation here is redundant since uv sync --locked --all-extras --dev in the Build step should install all development dependencies including cram. Consider removing the cram installation from this line.
| run: uv pip install pip cram | |
| run: uv pip install pip |
.github/workflows/ci.yml
Outdated
| run: | | ||
| $env:TERM = "dumb" | ||
| $env:TTY_INTERACTIVE = "0" | ||
| uv pip install cram |
There was a problem hiding this comment.
cram is being installed again here, but it should already be available from the uv sync --locked --all-extras --dev command in the Build step (line 46). This redundant installation should be removed. If cram is not being installed by uv sync, it suggests it's not properly declared in the project's dev dependencies, which should be fixed there instead.
| uv pip install cram |
|
@copilot look at the test failures; cram is not being found |
|
The main error is the regexp in |
This should fail until ucam-eo#110 is merged
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tmp.close() | ||
| country_gdf.to_file(tmp.name, driver="GeoJSON") | ||
| country_geojson_file = tmp.name | ||
|
|
There was a problem hiding this comment.
Calling tmp.close() explicitly inside the context manager is problematic. The file will be closed twice - once by the explicit close() call and again when exiting the with block. Instead, use the context manager's file handle and close it naturally, or restructure to close and open outside the context manager. Consider using delete=False with the context manager exiting naturally, then calling to_file() after the with block.
| tmp.close() | |
| country_gdf.to_file(tmp.name, driver="GeoJSON") | |
| country_geojson_file = tmp.name | |
| tmp_name = tmp.name | |
| country_gdf.to_file(tmp_name, driver="GeoJSON") | |
| country_geojson_file = tmp_name |
| Write-Host "$($item.Name)/" -ForegroundColor Cyan | ||
| } | ||
|
|
||
| $children = Get-ChildItem -Path $Path -ErrorAction SilentlyContinue | Sort-Object { -not $_.PSIsContainer }, Name |
There was a problem hiding this comment.
The property PSIsContainer is deprecated in PowerShell 7+. Consider using the PSIsDirectory property instead for future compatibility. This should be: Sort-Object { -not $_.PSIsDirectory }, Name
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
this will fail until #110 but checking it gets as far as the build