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

[proj4] Update to 8.0.0 #16494

Merged
merged 2 commits into from
Nov 19, 2021
Merged

Conversation

rhuijben
Copy link
Contributor

@rhuijben rhuijben commented Mar 2, 2021

Describe the pull request

  • What does your PR fix?
    Upgrade proj[4] to 8.0.0

Minimal change to bump the version. Removes the legacy (proj4 like) api, and therefore fixes some dependencies.

Originally this pull request contained some work to unconditonally enable tiff support, but this part is now removed. This same support is now independently(?) added to #20443 by @dg0yt.

This pull request (now open for half a year) re-opens the proj project for bumping to more recent versions (2021-11-15: last version 8.2.0)

  • Which triplets are supported/not supported? Have you updated the CI baseline?
    All existing triplets supported (Windows, Linux, OS/X, UWP)

  • Does your PR follow the maintainer guide?

@NancyLi1013 NancyLi1013 added the category:port-update The issue is with a library, which is requesting update new revision label Mar 2, 2021
@NancyLi1013 NancyLi1013 changed the title Feature/proj4 8.0.0 [proj4] Update to 8.0.0 Mar 2, 2021
@rhuijben
Copy link
Contributor Author

rhuijben commented Mar 2, 2021

With this release the base package was fixed to properly link curl in static mode.

Some of the depending packages have explicit rules on how to link against proj4 that don't include somewhat optional dependencies that are still required for full reprojection accuracy.

At compile time the code needs to know if it uses the TIFF component (as storage for gridshifts) or not, so this can't be a VCPKG feature knob if I read the code correctly.

I think we should enable the TIFF component unconditionally in the base port and perhaps add a slave port for PROJ4 without TIFF-gridshift support. I will see if I can fix the depending ports

@JackBoosY JackBoosY linked an issue Mar 31, 2021 that may be closed by this pull request
@rhuijben rhuijben force-pushed the feature/proj4-8.0.0 branch 2 times, most recently from 545c4b0 to bec82c1 Compare April 2, 2021 10:43
@rhuijben
Copy link
Contributor Author

rhuijben commented Apr 2, 2021

This should fix the build for libgeotiff and libspatialite for shared library builds. Next step would be checking static and static-md builds.

@rhuijben rhuijben force-pushed the feature/proj4-8.0.0 branch 2 times, most recently from f09398d to 382873a Compare April 4, 2021 10:19
@rhuijben rhuijben marked this pull request as ready for review April 4, 2021 10:20
@rhuijben
Copy link
Contributor Author

rhuijben commented Apr 4, 2021

The vtk port still depends on the legacy proj4 api that has been deprecated since proj 6.0, would have been removed by proj 7.0 and has finally been removed since proj 8.0.

Not sure what to do here. vtk probably needs a dependency on proj 7.2.1

@rhuijben rhuijben force-pushed the feature/proj4-8.0.0 branch 2 times, most recently from 17326fd to f90968b Compare April 6, 2021 07:33
@NancyLi1013
Copy link
Contributor

Seems need to merge master to resolve the conflicts. @rhuijben

@meastp
Copy link
Contributor

meastp commented May 6, 2021

I'm eagerly awaiting this version update. :) Consider updating to v8.0.1 while you're at it, which was released yesterday.

@dg0yt
Copy link
Contributor

dg0yt commented May 8, 2021

How about finally splitting port proj4 (legacy API) from port proj (modern API)?

@rhuijben
Copy link
Contributor Author

I'm eagerly awaiting this version update. :) Consider updating to v8.0.1 while you're at it, which was released yesterday.

I just committed proj 8.0.1 support to my vcpkg overlay repository on https://github.com/ampscm/vcpkg_additions

$ vcpkg upgrade --overlay-ports ..\vcpkg_additions\ports proj4:x64-windows-static-md --no-dry-run

@rhuijben
Copy link
Contributor Author

@NancyLi1013 What do we need to do with the VTK dependency that still assumes that it should use the proj4 apis that were deprecated since proj6 and now removed.

VTK has its own old version of proj4 included in its source, and as they recommend using that version there is no one working on fixing their support for newer proj versions.

@rhuijben
Copy link
Contributor Author

@dg0yt, @NancyLi1013 I like your suggestion on splitting between legacy [proj4] compatible and moving to the actual project name [proj] for future versions, but I think that decision should be taken by the VCPKG maintainers.

@dg0yt
Copy link
Contributor

dg0yt commented May 11, 2021

Grepping for " proj" in ports/\*/CONTROL and for '"proj' in ports/\*/vcpkg.json, no port depends on "proj", all depend on "proj4":

  • gdal
  • libgeotiff
  • libosmium
  • libspatialite
  • spatialite-tools
  • vtk

Consistency requirements (within vcpkg):

  • gdal also depends on libgeotiff, and gdal[libspatialite] depends on libspatialite.
  • spatialite-tools also depends on libspatialite.
  • vtk[all] uses gdal.
  • vtk is used by a number of other packages (itk, opencv*, paraview, pcl), but not explicitly on vtk[all] AFAICT.

What vcpkg users need is another question. But there are manifests, overlay ports and registries now.

@dg0yt
Copy link
Contributor

dg0yt commented May 11, 2021

PROJ 8 should also bring a pc file for pkg-config.

@JackBoosY
Copy link
Contributor

@NancyLi1013 What do we need to do with the VTK dependency that still assumes that it should use the proj4 apis that were deprecated since proj6 and now removed.

VTK has its own old version of proj4 included in its source, and as they recommend using that version there is no one working on fixing their support for newer proj versions.

Therefore, I think we should update proj4 and vtk together.

@dg0yt
Copy link
Contributor

dg0yt commented May 12, 2021

VTK has been updated: https://gitlab.kitware.com/vtk/vtk/-/issues/18130

@rhuijben
Copy link
Contributor Author

No changes, just force pushed in attempt to get through the issue that opencv is stuck downloading raw patchfiles from github.

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Thanks!

@NancyLi1013
Copy link
Contributor

Thanks for your efforts and patience. @rhuijben

Have you tested these features in this PR?

@NancyLi1013 NancyLi1013 added the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label Nov 16, 2021
@rhuijben
Copy link
Contributor Author

rhuijben commented Nov 16, 2021

Yes @NancyLi1013 , I tested the features of proj. I haven't tested it in the depending projects but depend on the proj version compatibility there. The vtk changes were tested in the vtk project itself as the patch(commit) is directly extracted from their git repository.

I still need the tiff support in my own static builds for 'good' accuracy, but that part of the original patch is now handled by a separate patch from @dg0yt which I will follow closely. (Feel free to ask if you need help somewhere @dg0yt. My original patch for other projects is still in my vcpkg fork on github)

@NancyLi1013
Copy link
Contributor

Thanks for your above information. @rhuijben Unfortunately, there are some new changes merged to master. So could you please resolve the conflicts?

@NancyLi1013 NancyLi1013 added requires:author-response and removed requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function labels Nov 17, 2021
@rhuijben
Copy link
Contributor Author

@NancyLi1013 thanks for the heads up. I just rebased on master. It looks like the Cairo part of the vcpkg patch was merged upstream, so I just had to shrink the patch to just the proj part to get things working again.

@nirvn
Copy link
Contributor

nirvn commented Nov 17, 2021

@rhuijben , out of curiosity, why not going for PROJ 8.1.1?

@rhuijben
Copy link
Contributor Author

@rhuijben , out of curiosity, why not going for PROJ 8.1.1?

Other packages may depend on explicit versions of proj.

And getting the dependencies fixed against >= 8.0.0 to allow future further updates is of higher priority than getting to the best version.

This patch gets us to 8.0.0 and then patches to 8.0.1, 8.1.0 and 8.1.1 will be trivial to get merged later on.

(The last version is 8.2.0, but that version asserts in debug mode in my personal testing setup... So I wouldn't recommend using that version until 8.2.1 is released)

@dg0yt
Copy link
Contributor

dg0yt commented Nov 17, 2021

I think it would be okay to jump to 8.0.1 immediately. But in any case, we need to the vtk patch finally merged. Other port updates are much easier.

@nirvn
Copy link
Contributor

nirvn commented Nov 17, 2021

@rhuijben , I wouldn't recommend 8.2.0 either. In any case, big thanks for your work here.

@rhuijben
Copy link
Contributor Author

I think it would be okay to jump to 8.0.1 immediately. But in any case, we need to the vtk patch finally merged. Other port updates are much easier.

Good point. If I have to rebase it again I will bump to 8.0.1. With a bit of luck this last one might get through this week though.

@rhuijben
Copy link
Contributor Author

@NancyLi1013 Testing is all green now. Only change vs yesterday is that the patch file to make mapnik find proj is now smaller.

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Nov 18, 2021
@vicroms vicroms merged commit 56e89eb into microsoft:master Nov 19, 2021
if(USE_PROJ)
#https://proj.org/development/cmake.html
- mapnik_find_package(PROJ ${PROJ_MIN_VERSION} QUIET)
+ mapnik_find_package(PROJ REQUIRED)
Copy link
Contributor

@mathisloge mathisloge Nov 19, 2021

Choose a reason for hiding this comment

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

This is strange but ok for mapnik with vcpkg.

I'll investigate that upstream why and what fails. Maybe some version thingy in the config file of proj is requiring an exact match.

Thanks for making the proj8 update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FindProj not working as expected
8 participants