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.

Sign up for GitHub

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

Jump to bottom

[gdal] Fix build for non-Windows targets #17698

Merged
merged 24 commits into from Jul 23, 2021
Merged

[gdal] Fix build for non-Windows targets #17698

merged 24 commits into from Jul 23, 2021

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented May 6, 2021 •
edited

  • What does your PR fix?

    This PR fixes building gdal for non-Windows systems ([GDAL] On Linux completly broken #9068).
    It also restructures the portfile for better maintainability.
    It includes significant patching to configure.ac. There seems to be no better way to handle vcpkg's release+debug filesystem layout.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all, no

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    yes

@JackBoosY JackBoosY self-assigned this May 6, 2021
@JackBoosY JackBoosY added the category:port-bug The issue is with a library, which is something the port should already support label May 6, 2021
@dg0yt dg0yt mentioned this pull request May 7, 2021
Closed
@dg0yt dg0yt mentioned this pull request May 21, 2021
Merged
@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 19, 2021

Note to self: There are some conflicting gdal changes in #15808 (draft) by @ras0219

@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 30, 2021

Still waiting for approval of curl changes (#17790).
The pc file and the cmake wrapper need extra work.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 5, 2021

TODO: gdal.pc

FTR, CI doesn't build many depending port at the moment:

  • osx: none [!]
  • linux: osg, osg-qt
  • windows: osg, osg-qt, pdal, pdal-c
  • x64_windows_static_md: osg, osg-qt, pdal

(And no gdal build for arm, uwp.)

@dg0yt dg0yt mentioned this pull request Jul 6, 2021
Merged
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 12, 2021 •
edited

Ping @JackBoosY. GIthub states that I "marked this pull request as ready for review 2 days ago".

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 12, 2021

PS: I will fix the LIBRARY typo together with possible review comments.

@JackBoosY
Copy link
Contributor

cc @Neumann-A for review this PR.

@JackBoosY JackBoosY added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jul 12, 2021
@dg0yt dg0yt mentioned this pull request Jul 18, 2021
Merged
8 tasks
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 20, 2021

Kindly reminder for reviews. Cannot look at gdal updates, or upstreaming any changes, before sufficient agreement on the current PR. And a huge number of dependencies makes this PR volatile until merged.

@JackBoosY
Copy link
Contributor

Can you please also fix #17609?
See my comment.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 20, 2021

Can you please also fix #17609?

Hm, it is somewhat out of scope. This PR is mostly about the autotools build system, apart from general port file improvements.

#17609 is reported for Windows. The comments are using CMake language, but GDAL isn't.

And I don't have the MSVC toolchain. I cannot build gdal[core,libspatialite]:x64-windows outside CI.

@JackBoosY
Copy link
Contributor

@dg0yt Okay, that's fine.

@JackBoosY
Copy link
Contributor

I'd like @Neumann-A review this PR.

@JackBoosY
Copy link
Contributor

Does this issue also fix #15526?

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 21, 2021

After re-reading, #15526 is about using (static) gdal with MSVC. The whole area of using gdal will probably need more work. It depends on whether usage requirements are handled

  • via pkgconfig files
  • FindGDAL.cmake (certainly incomplete)
  • manually (in MSVC projects?).

As I wrote in #17698 (comment), there is a varying level of CI coverage for using gdal in different triplets. I guess there are also lots of workaround in consuming ports. Improving this situation is for coming PRs. The focus of this PR is on controlling what is pulled in when building gdal, thus stabilizing what needs to be pulled in when linking statically to gdal.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jul 21, 2021
@dg0yt dg0yt mentioned this pull request Jul 21, 2021
Closed
@mathisloge
Copy link
Contributor

gdal 3.3.1 is released, too. https://github.com/OSGeo/gdal/releases/tag/v3.3.1

@mathisloge
Copy link
Contributor

@dg0yt have tested gdal on windows with msvc (dynamic amd x64).
This PR has also fixed some critical issues for me on windows. great work!

strega-nil-ms
strega-nil-ms approved these changes Jul 22, 2021
View reviewed changes
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

Just needs the minor change

ports/gdal/portfile.cmake Outdated Show resolved Hide resolved
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 23, 2021

gdal 3.3.1 is released, too. https://github.com/OSGeo/gdal/releases/tag/v3.3.1

I'm aware of that. But I can't push new things while waiting for review. This PR already triggered countless work on incoming dependencies.

vicroms
vicroms approved these changes Jul 23, 2021
View reviewed changes
Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

LGTM waiting for CI results

@strega-nil-ms strega-nil-ms merged commit 4990d2b into microsoft:master Jul 23, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 23, 2021

I verified that find_package(GDAL REQUIRED) broken now, due to the chances on the last mile, #17698 (comment). Next time I will revert to draft.

@mathisloge
Copy link
Contributor

mathisloge commented Jul 23, 2021 •
edited

@dg0yt yep just runned into it at #18849 (with feature input-gdal)

@dg0yt dg0yt mentioned this pull request Jul 23, 2021
Merged
@JackBoosY JackBoosY mentioned this pull request Jul 26, 2021
Closed
@Osyotr Osyotr mentioned this pull request Sep 3, 2021
Merged
@dg0yt dg0yt deleted the gdal-vcpkg branch August 16, 2023 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@strega-nil-ms strega-nil-ms strega-nil-ms approved these changes

@vicroms vicroms vicroms approved these changes

@JackBoosY JackBoosY JackBoosY approved these changes

Assignees

@JackBoosY JackBoosY

Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dg0yt @JackBoosY @mathisloge @vicroms @strega-nil-ms @strega-nil