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

build: Remove compatibility to GDAL 1 and <2.2 #2995

Merged
merged 23 commits into from
Dec 20, 2023

Conversation

lbartoletti
Copy link
Contributor

This PR aims to cleanup grass code to remove "dead code" since most modern distribution/packaging have GDAL 3.

Fixes #2645

@wenzeslaus
Copy link
Member

I think we had some confusion about the test failures at the in-person meeting, but these are real, although I don't quite see a useful message:

...
Building areas...
0..33..66..100
Attaching islands...
0..33..66..100
-----------------------------------------------------
Finding centroids for OGR layer <all_types>...
ERROR: Unable to import OGR datasource <data/all_types_wgs84.gpkg>
... 

@lbartoletti
Copy link
Contributor Author

I think we had some confusion about the test failures at the in-person meeting, but these are real, although I don't quite see a useful message:

...
Building areas...
0..33..66..100
Attaching islands...
0..33..66..100
-----------------------------------------------------
Finding centroids for OGR layer <all_types>...
ERROR: Unable to import OGR datasource <data/all_types_wgs84.gpkg>
... 

Was a mistake when I've manually deleted some parts of code in v.in.ogr/main.c. It should be fixed in https://github.com/OSGeo/grass/pull/2995/commits/7d45b62c92d63b2357066ebb6b8f472fb8c8b00b

@wenzeslaus wenzeslaus changed the title [GDAL] Remove compatibility to GDAL 1 and <2.2 build: Remove compatibility to GDAL 1 and <2.2 Jun 6, 2023
@wenzeslaus
Copy link
Member

Probably an ancient GDAL in our CentOS 7 build is causing the build failure. That was exactly the reason to have CentOS 7 to catch cases for these outdated dependencies. However, perhaps now it the time to finally remove our CentOS 7 build. (It anyway uses conda to get things working, so it is not pure CentOS anyway.)

@neteler neteler added this to the 8.4.0 milestone Aug 16, 2023
@neteler
Copy link
Member

neteler commented Oct 31, 2023

@lbartoletti would you mind to rebase this PR?

@lbartoletti
Copy link
Contributor Author

@lbartoletti would you mind to rebase this PR?

done.

@neteler
Copy link
Member

neteler commented Nov 2, 2023

@metzm , others: any objections to merge this PR?

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

@metzm , others: any objections to merge this PR?

I'd welcome this simplification of the code! I noticed some places where calls to GDAL are defined as macros / typedef depending on version. Those may probably be removed all together and be replaced by directly using the GDAL API. I made a comments in-line for the cases which could be relevant for this change.

vector/v.external/local_proto.h Show resolved Hide resolved
vector/v.in.ogr/global.h Show resolved Hide resolved
vector/v.out.ogr/local_proto.h Show resolved Hide resolved
@metzm
Copy link
Contributor

metzm commented Dec 2, 2023

Very nice, no objections from my side to drop support for those old GDAL versions.

The suggestions of @nilason should be applied before merging. All macros and typedefs related to GDAL versions before 2.2 can go.

@lbartoletti
Copy link
Contributor Author

Very nice, no objections from my side to drop support for those old GDAL versions.

The suggestions of @nilason should be applied before merging. All macros and typedefs related to GDAL versions before 2.2 can go.

@nilason @metzm It's done in my last commit, and I apologize for my late reaction.

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

Thanks @lbartoletti ! Looks good to me.

(This removes some 500 lines of obsolete code!)

@wenzeslaus
Copy link
Member

This removes some 500 lines of obsolete code!

Thanks! That's really great!

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Issues raised by @nilason and further elaborated on by @metzm were addressed. Compatibility wrappers are removed from the headers which ensures ensures there are not used elsewhere.

There should be no impact on user code even with the change in gis.h if it is already portable code.

I assume these are all occurrences of GDAL_VERSION_NUM in the source code, but even if they are not, they are easy to find and would not harm this great PR.

@wenzeslaus wenzeslaus self-assigned this Dec 20, 2023
@wenzeslaus wenzeslaus merged commit ea8f3ea into OSGeo:main Dec 20, 2023
18 checks passed
tmszi pushed a commit to tmszi/grass that referenced this pull request Dec 20, 2023
This basically remove "dead code" since most modern distribution/packaging have GDAL 3.

Version of GDAL no longer supported: 1 and <2.3. Remove compatibility macros and use GDAL functions directly.

Fixes OSGeo#2645.
HuidaeCho pushed a commit to HuidaeCho/grass that referenced this pull request Jan 9, 2024
This basically remove "dead code" since most modern distribution/packaging have GDAL 3.

Version of GDAL no longer supported: 1 and <2.3. Remove compatibility macros and use GDAL functions directly.

Fixes OSGeo#2645.
@lbartoletti lbartoletti deleted the gdal_remove_old_code branch January 19, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] Remove checks for GDAL v1 and v2
5 participants