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

GDAL 3.7.2 build #120

Merged
merged 11 commits into from
Nov 6, 2023
Merged

GDAL 3.7.2 build #120

merged 11 commits into from
Nov 6, 2023

Conversation

pomadchin
Copy link
Member

I had a bit of time to hack on the GDAL update.

This PR updates builds to be against GDAL 3.7.2. It also contains some minor test adjustments.

I think we can still use CircleCI for releases.

It's very much WIP; feel free to rush in and introduce more changes / improvements. Not tested on Mac and Windows.

Closes #100

Comment on lines +171 to +174
BOOST_AUTO_TEST_CASE(destroy)
{
GDALDestroyDriverManager();
}
Copy link
Member Author

@pomadchin pomadchin Oct 29, 2023

Choose a reason for hiding this comment

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

We were lucky or that's the GDAL behavior change; without it tests fail.

Added it in all cases we wan't to undo GDALAllRegister() calls.

# failure example
# cd .github/workflows; docker compose -f ../docker-compose.yml up build-linux-amd64
github-build-linux-amd64-1  | corrupted size vs. prev_size
github-build-linux-amd64-1  | Aborted
github-build-linux-amd64-1  | make[1]: *** [Makefile:16: tests] Error 134
github-build-linux-amd64-1  | make[1]: Leaving directory '/workdir/src/unit_tests'
github-build-linux-amd64-1  | make: *** [Makefile:39: tests] Error 2
github-build-linux-amd64-1  | make: Leaving directory '/workdir/src'

@@ -55,7 +55,7 @@ BOOST_AUTO_TEST_CASE(get_block_size)

ld.get_block_size(locked_dataset::WARPED, 1, &width, &height);
BOOST_TEST(width == 512);
BOOST_TEST(height == 128);
BOOST_TEST(height == 512);
Copy link
Member Author

@pomadchin pomadchin Oct 29, 2023

Choose a reason for hiding this comment

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

So weird it's been 128 before.

For the context: for "-co", "BLOCKXSIZE=512", "-co", "BLOCKYSIZE=512" input warp params I would expect both width and height to be 512.

Choose a reason for hiding this comment

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

I also saw this error, but I didn't have time to find the source. Thanks.

Comment on lines 452 to 454
auto expected = std::vector<double>{
-8915910.5905594081, 33.88424960091178, 0,
5174836.3438357478, 0, -33.88424960091178}; // Manually verified
-8915910.5905594081, 33.88424960091165, 0,
5174836.343835746, 0, -33.88424960091165}; // Manually verified
Copy link
Member Author

Choose a reason for hiding this comment

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

Angry doubles.

Choose a reason for hiding this comment

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

Real-valued comparisons should prefer to use a test of the form abs(target - actual) < EPSILON for some appropriately set EPSILON.

Copy link
Member Author

@pomadchin pomadchin Oct 31, 2023

Choose a reason for hiding this comment

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

yea, I wanted to minimize changes; but agree; reverted these numbers change and added a better comparison: f1a03c9

@pomadchin pomadchin force-pushed the upd/gdal-3.7.x branch 2 times, most recently from b653077 to 1b68890 Compare October 29, 2023 02:24

services:
# cd .github/workflows; docker compose -f ../docker-compose.yml up build-linux-amd64
build-linux-amd64:
Copy link
Member Author

@pomadchin pomadchin Oct 29, 2023

Choose a reason for hiding this comment

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

Such a winner for the local development; + mb in a separate PR this can be extended to work with GitHub Actions; (just a retranslation of the CircleCI yaml file into the docker-compose shape)

The pattern is similar to what we've done in the GeoTrellis repo; the same docker-compose used for the local development and for the CI purposes; super convenient!

rm -f libgdal-3.1.2-hd7bf8dc_4.tar.bz2
wget "https://anaconda.org/conda-forge/libgdal/3.7.2/download/osx-64/libgdal-3.7.2-h85f6614_6.conda" && \
mkdir -p gdal/3.7.2 && \
cph extract libgdal-3.7.2-h85f6614_6.conda --dest gdal/3.7.2 && \
Copy link
Member Author

@pomadchin pomadchin Oct 29, 2023

Choose a reason for hiding this comment

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

conda doesn't ship tar.bz2 archives anymore but uses it's own tar.bz2-like packaging => so I had to install conda to extract it 🤦

https://github.com/conda/conda-package-handling

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could turn these version strings into buildargs

Copy link
Member Author

@pomadchin pomadchin Nov 5, 2023

Choose a reason for hiding this comment

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

I went mb a bit too far :D d0f5828#diff-93c714fe4d67d93f9c74b0413dc5d5b7975e04602a2ce531ac5f5c4034e5f3fcR4-R9

Two visible cons: MacOS and Windows 'GDAL versions' are not really versions, but achive names. Still not a too bad result it looks like. For linux builds its a really nice parametrization.

@pomadchin pomadchin requested a review from echeipesh October 29, 2023 02:47
@jamesmcclain
Copy link
Member

🙏 ❤️

Thanks, @pomadchin , I'll take a look at this in the upcoming week. Please ping me if I haven't done so by Wednesday or Thursday .

Copy link
Member

@jamesmcclain jamesmcclain left a comment

Choose a reason for hiding this comment

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

LPGTM 👍

rm -f libgdal-3.1.2-hd7bf8dc_4.tar.bz2
wget "https://anaconda.org/conda-forge/libgdal/3.7.2/download/osx-64/libgdal-3.7.2-h85f6614_6.conda" && \
mkdir -p gdal/3.7.2 && \
cph extract libgdal-3.7.2-h85f6614_6.conda --dest gdal/3.7.2 && \
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could turn these version strings into buildargs

@jamesmcclain
Copy link
Member

image

We've got the whole crew in here 😎

@pomadchin
Copy link
Member Author

pomadchin commented Nov 5, 2023

Merging this one on Monday; LMK if there are any concerns / further improvements to do before I click the button!

@pomadchin pomadchin merged commit fd0cc7d into geotrellis:master Nov 6, 2023
@pomadchin pomadchin changed the title GDAL 3.7.2 GDAL 3.7.2 build Nov 7, 2023
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.

Update to GDAL 3.7.x
3 participants