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

[Feat] Remove checks for GDAL v1 and v2 #2645

Closed
wenzeslaus opened this issue Nov 10, 2022 · 7 comments · Fixed by #2995
Closed

[Feat] Remove checks for GDAL v1 and v2 #2645

wenzeslaus opened this issue Nov 10, 2022 · 7 comments · Fixed by #2995
Labels
C Related code is in C enhancement New feature or request

Comments

@wenzeslaus
Copy link
Member

Is your feature request related to a problem? Please describe.

We have 76 occurrences of GDAL_VERSION_NUM check for C preprocessor if statements spread over 21 files. Most of these check are for v1 and v2 of GDAL. The code would be simpler without them.

Describe the solution you'd like

Remove all checks for v1 and v2. Leave only code for new versions for cases where there is code for old and new version. That is, assume most recent GDAL v2.

Use GDAL_COMPUTE_VERSION for v3 checks (4 places) instead of the computed number. GDAL_COMPUTE_VERSION is defined at the same place in GDAL as GDAL_VERSION_NUM (gdal_version.h) and it is used in the Vector API tutorial.

Describe alternatives you've considered

Remove checks for v3 too (all are 3.0.0), require v3 as minimum. Perhaps too big a jump. There is enough v2 checks to be removed to make a difference while keeping the compatibility high.

Removing only the v1 checks and leaving the v2 checks there: GRASS GIS would still compile with some undefined version of v1, but would assume the features are there. I think there is no need to support GDAL 1 in GRASS GIS 8.3, so I would just assume in the code it's some latest release from the v2 series.

Don't remove any of the checks, but just use GDAL_COMPUTE_VERSION for clarity. This adds readability, but does not simplify the overall code structure.

Additional context

Resulting simplified code may resolve a GNU indent issue ./vector/v.in.ogr/main.c:1457: Error:Stmt nesting error reported in #1630.

In CI we are mostly using Ubuntu 20.04 which has GDAL 3.0.4.

The search (related to the numbers above) can be done with:

grep --color=auto --exclude-dir={.git,OBJ.*,locale,dist.*,bin.*} -IrnE GDAL_VERSION_NUM | grep "GDAL_VERSION_NUM [<=>]" | grep "\#if" | wc -l
grep --color=auto --exclude-dir={.git,OBJ.*,locale,dist.*,bin.*} -IrnE GDAL_VERSION_NUM | grep "GDAL_VERSION_NUM [<=>]" | grep "\#if" | sed 's+:.*++g' | sort | uniq | wc -l
grep --color=auto --exclude-dir={.git,OBJ.*,locale,dist.*,bin.*} -IrnE GDAL_VERSION_NUM | grep "GDAL_VERSION_NUM [<=>]" | grep "\#if" | grep " 3" | wc -l
@wenzeslaus wenzeslaus added enhancement New feature or request C Related code is in C labels Nov 10, 2022
@nilason
Copy link
Contributor

nilason commented Nov 10, 2022

https://repology.org/project/gdal/versions can help on making a decision.

@wenzeslaus
Copy link
Member Author

Does someone know how to make a relevant query on Repology or how to efficiently evaluate what is there?

@nilason
Copy link
Contributor

nilason commented Dec 2, 2022

In general I think we should be a bit more “pushy” when it comes to issues like this. Maybe I’m missing something essential, in #nix world, if so forgive my ignorance. GDAL 3 was released a bit more than 3 years ago. If an OS version still doesn’t support GDAL 3, why expect it to run GRASS 8.2+? I vote for dropping support for GDAL 1 & 2.

@nilason
Copy link
Contributor

nilason commented Dec 2, 2022

Looking at the Repology list, GDAL 1 is hardy represented at all. Yet GRASS 8 is supposed to support it. Why? This underlines my previous statement that we need to be more forward looking.

Suggestion - news item for 8.3 release: GDAL 3+ is a requirement.

@marisn
Copy link
Contributor

marisn commented Dec 2, 2022

Maybe I’m missing something essential, in #nix world, if so forgive my ignorance.

Yes, you are. It is common to compile GRASS from sources on HPC systems and at the same time HPC systems not always are up to date. Thus supporting older versions is a nice thing to have.

At the same time, I'd say +1 for removal of GDAL 1.x.
Dropping GDAL 2.x also seems to be doable, as even previous Ubuntu LTS lists 3.0 version as being available.

@wenzeslaus
Copy link
Member Author

...It is common to compile GRASS from sources on HPC systems and at the same time HPC systems not always are up to date. Thus supporting older versions is a nice thing to have.

For the record, I need to use conda on our HPC anyway because there is no GDAL. I actually ended up using conda for dependencies, so really no need for old GDAL there.

@lbartoletti
Copy link
Contributor

According to repology almost every distribution uses GDAL 3. Only Debian 10 or old Fedora and openSuse have GDAL 2. However, these distributions have a "stable" packaging, so Grass 8.4 will not be packaged on. For me, it's safe to remove this old code.

wenzeslaus pushed a commit that referenced this issue 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 #2645.
tmszi pushed a commit to tmszi/grass that referenced this issue 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 issue 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants