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: update proj dependency #94807

Closed
wants to merge 14 commits into from
Closed

Conversation

danielnachun
Copy link
Member

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@BrewTestBot BrewTestBot added the python Python use is a significant feature of the PR or issue label Feb 10, 2022
@carlocab carlocab added long build Needs CI-long-timeout CI-long-timeout Use longer GitHub Actions CI timeout. labels Feb 10, 2022
@carlocab
Copy link
Member

Dependencies libgeotiff and libspatialite need to also be migrated. Along with everything that depends on gdal.

@carlocab carlocab removed the CI-long-timeout Use longer GitHub Actions CI timeout. label Feb 10, 2022
@danielnachun danielnachun added the CI-long-timeout Use longer GitHub Actions CI timeout. label Feb 10, 2022
@danielnachun danielnachun removed the CI-long-timeout Use longer GitHub Actions CI timeout. label Feb 10, 2022
@danielnachun
Copy link
Member Author

We're in a bit of bind here. I was able to migrate all dependents of proj@7 to proj except 3:

  • liblwgeom - deprecated, appears to be superseded by librttopo
  • osm2pgsql - this project appears to be active (the last release was 16 days ago). We should open an upstream issue to inquire about PROJ 8 support
  • spatialite-gui - this is where our remaining dependency conflict comes from, and it's not pretty. The last stable version doesn't support PROJ 8, but the 2.1 beta does. I tried to build the 2.1 beta just to see if would work, and it fails because it needs librasterlite2, which we don't package. Our librasterlite formula is for an older library which is no longer developed because it has been superseded by librasterlite2. Potentially we could just update the librasterlite formula because it has no dependents. But that doesn't help with version 2.1 of spatialite-gui still being in beta.

Any thoughts on the best solution here?

@carlocab
Copy link
Member

We can probably update librasterlite to librasterlite2, but I'd double check what other package managers/distros have done about this.

For spatialite-gui, we should ask upstream if they have a timeline for 2.1 to come out of beta.

@danielnachun
Copy link
Member Author

I've looked into how Linux distros have handled librasterlite/librastlerlite2 and it appears that both Debian and Fedora removed librasterlite and replaced it with librasterlite2 some time ago. So it seems that we are actually behind the times and should probably do the same. It seems like a formula rename would make sense to match what other distros do.

I was looking at the project timeline for spatialite-gui and there seems to be quite recent development activity: https://www.gaia-gis.it/fossil/spatialite_gui/timeline. However, the last stable version (1.7.1, which we are using) was released in 2013. The version 2.0.0-devel was released in 2015, and since then there's been a 2.1.0-beta0 and 2.1.0-beta1. I think we should clarify with upstream why this project isn't following making stable releases before going to the next beta.

@carlocab
Copy link
Member

It could be that they just have a stricter definition of "stable" than the one we usually use. It might mean "bug fixes only, no changes at all to the API", which is not uncommon.

@carlocab
Copy link
Member

Debian and Fedora ship spatialite-gui 2.1.0-beta. I think we can do the same.

@GeraldCNelson
Copy link

For what it is worth, here's the librasterlite website - https://www.gaia-gis.it/fossil/librasterlite/index. Two things to note. 1. is that it is no longer maintained and advises using librasterlite. 2. librasterlite and spatialite are done by the same author. spatialite guide website is here - https://www.gaia-gis.it/fossil/libspatialite/index. spatiallite proper is here - https://www.gaia-gis.it/fossil/libspatialite/index. Its current version is 5.01 released in 2021.

@danielnachun
Copy link
Member Author

Debian and Fedora ship spatialite-gui 2.1.0-beta. I think we can do the same.

I was leaning towards doing that. I'm going to formula rename of librasterlite to librasterlite2, update it to the latest version, and update spatialite-gui to 2.1.0-beta.

I'm also going to disable proj support in osm2pgsql because they recently added an option to do this https://github.com/openstreetmap/osm2pgsql/pull/1331/files, and apparently libosmium, which is upstream to osm2pgsql, considers PROJ support for libosmium deprecated: osmcode/libosmium#295. This will leave only liblwgeom as a proj@7 dependent, and as mentioned above, liblwgeom is itself deprecated.

@danielnachun
Copy link
Member Author

On second thought, maybe it's better to create librasterlite2 as a new formula? The only problem I see here is that it also is a "beta", but like spatialite-gui is treated as a stable package by Debian and Fedora who both ship version 1.1.0-beta1.

@danielnachun
Copy link
Member Author

danielnachun commented Feb 12, 2022

I was able to successfully build both librasterlite2 and spatialiate-gui using proj locally for macOS and Linux. However, while doing this testing I discovered a number of other issues that need to be fixed before I think we can move further with this:

  • It turns out libgaiagraphics actually doesn't work with proj. It found proj@7 through opportunistic linking on macOS, but failed in the Linux VM because proj@7 wasn't installed. However, the upstream page for libgaiagraphics explicitly states it is deprecated and its replacement is... librasterlite2. So instead of migrating libgaiagraphics from proj@7, I will deprecate it. Its only dependent was spatiliate-gui, which now uses librasterlite2 instead, so we don't have to worry about the deprecation causing issues for any other dependents.
  • spatialiate-tools was unbottled on Linux because it has an unbottled dependecy readosm, which had a broken test on Linux, and because it appends -liconv unconditionally to LDFLAGS, when this should be macOS-only. I've opened readosm: fix build on Linux #94995 to fix readosm (which of course had some other issues too, though they were fixable), and I have a PR ready to fix spatialite-tools once readosm is bottled on Linux.
  • mapserver was unbottled on Linux because it needs to be built with GCC 11. I have opened mapserver: fix Linux install #94997 to fix this.
  • postgis was unbottled on Linux because it also needs to be built with GCC 11, and there may also be an issue because has a weird install process after compilation that installs to staging directory instead of the keg, and then manually copies over some files. I'm working on a fix for this now that is hopefully less messy.

Given how deeply this PR has dragged us into dependency hell, I'm inclined now to try to get librasterlite2 merged and spatialite-gui updated while still having them build against proj@7, and then rebase this PR once all these other issues are fixed so that it can just be focused on the dependency migration.

UPDATE All of the above issues have been fixed except spatialite-gui, which needs librasterlite2 to be added, and it needs to be updated to the latest version. Once that is done, I can rebase this PR and the dependency conflicts will all be solved.

@danielnachun
Copy link
Member Author

As suggested in #95481, we will update spatialite-gui in this PR rather than doing it separately. A few more updates:

  • It turns out that osm2pgsql does support proj, but defaults to using proj@7 if it finds it. That caused it to fail to build due to opportunistic linking on my local installation. In CI this won't happen, but osm2pgsql has added a USE_PROJ_LIB CMake flag that I have set to always use the newer PROJ API even if the old one is detected.
  • We may have to disable libgaiagraphics here or in a separate PR, because it has what I believe is an unresolvable dependency conflict. It has been deprecated upstream because it has been replaced by librasterlite2 and will never be updated to support proj. However, it depends on libgeotiff, which does support proj and is being updated in this PR. We could add it the list of allowable dependency conflicts, but it could be broken and that seems as bad if not worse than just disabling it.
  • librasterlite is not part of the dependency tree here (although it is a dependent of libgeotiff and libspatialite, but it should still be deprecated in a separate PR, because has also been replaced by librasterlite2 and will not be updated if it has future dependency issues.

mkdir "build" do
system "cmake", "-DWITH_LUAJIT=ON", "..", *std_cmake_args
system "cmake", "-DWITH_LUAJIT=ON", "-DUSE_PROJ_LIB=6", "..", *std_cmake_args
Copy link
Member

Choose a reason for hiding this comment

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

Where does the 6 come from?

Copy link
Member Author

Choose a reason for hiding this comment

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

As described here: osm2pgsql-dev/osm2pgsql@552e4bc, the old API for PROJ was actually implemented in PROJ 4, and the new API debuted in PROJ 6. Both APIs were supported in PROJ 6 and 7, but in PROJ 8, the old PROJ API was removed. So the number here is referring the version in which the API was first implemented, not the version currently being used.

@cho-m
Copy link
Member

cho-m commented Mar 2, 2022

May need to check compatibility with PROJ 9, which will be merged with #96103

@danielnachun
Copy link
Member Author

I'm going to disable libgaiagraphics because I don't believe there's a way us to ship a working bottle anymore, and I'll rebase this against PROJ 9. I expect it should work, as the underlying API does not appear to have fundamentally changed since PROJ 6.

@danielnachun danielnachun added CI-long-timeout Use longer GitHub Actions CI timeout. and removed CI-long-timeout Use longer GitHub Actions CI timeout. labels Mar 26, 2022
@danielnachun
Copy link
Member Author

It looks like the only thing needed for librasterlite to build was an explicit dependency on proj@7. I think we're good to go!

Comment on lines 8 to 9
livecheck do
url "https://www.gaia-gis.it/gaia-sins/spatialite-gui-sources/"
Copy link
Member

Choose a reason for hiding this comment

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

In separate PR, we may want to relax livecheck regex to allow for -beta versions to avoid warnings until 2.x is out of beta.

sha256 "37f71f3cb2b0b9649eb85a51296187b0adf2972c5a1d3ee0daf3082e2c35025e"
end
uses_from_macos "curl"
uses_from_macos "libxml2"
Copy link
Member

Choose a reason for hiding this comment

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

Not too important, but Homebrew's libxml2 is getting linked rather than using macOS version. Probably pulled in via libspatialite.

Shouldn't be an issue as indirect dependency avoids broken linkage. In future, may want to check if libspatialite truly needs a newer copy of libxml2 than what is provided by system.

Copy link
Member

@cho-m cho-m left a comment

Choose a reason for hiding this comment

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

I don't see any issues blocking merge.

Can wait for @carlocab to also take a look.

@cho-m cho-m added the ready to merge PR can be merged once CI is green label Mar 27, 2022
@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

carlocab added a commit to carlocab/homebrew-core that referenced this pull request Mar 27, 2022
@carlocab carlocab mentioned this pull request Mar 27, 2022
6 tasks
BrewTestBot pushed a commit that referenced this pull request Mar 27, 2022
This is needed after #94807.

Closes #97897.

Signed-off-by: Sean Molenaar <1484494+SMillerDev@users.noreply.github.com>
Signed-off-by: BrewTestBot <1589480+BrewTestBot@users.noreply.github.com>
@danielnachun danielnachun deleted the gdal branch April 4, 2022 22:55
@github-actions github-actions bot added the outdated PR was locked due to age label May 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request long build Needs CI-long-timeout outdated PR was locked due to age python Python use is a significant feature of the PR or issue ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants