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.1.1, revise features and dependencies #20443

Merged
merged 16 commits into from
Dec 10, 2021

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Sep 30, 2021

  • What does your PR fix?

    • Update proj to 8.1.1. (Resolves [proj4] update to 8.1.1 #19910.)
    • Adds a new net feature. It is enabled by default, following upstream.
      Network support (via libcurl) was bound to the tools feature but in fact it is independent.
      (I wonder if there is an established name for such a feature. Alternatives are "network" or "online".)
    • Enables tiff and network support for static builds. This is essential for the official osx and linux triplets.
      The key issue was properly exporting the dependencies for consumers (cmake config and pkgconfig).
  • Which triplets are supported/not supported? Have you updated the CI baseline?

    unchanged

  • 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

Important behavioural change

The opt-in feature tools no longer implies network support.

@dg0yt dg0yt changed the title [proj] Revise features and dependencies [proj4] Revise features and dependencies Sep 30, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 30, 2021

Okay, libspatialite needs to learn transitive dependencies.

@JackBoosY JackBoosY added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Oct 8, 2021
Comment on lines 16 to 22
if("net" IN_LIST FEATURES)
vcpkg_list(APPEND TOOL_NAMES projsync)
set(feature_for_projsync "tools")
else()
set(VCPKG_BUILD_SHARED_LIBS OFF)
set(TOOL_NAMES cct cs2cs geod gie proj projinfo)
set(feature_for_projsync "net")
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally don't like this, this will increase variability.

Copy link
Contributor Author

@dg0yt dg0yt Oct 8, 2021

Choose a reason for hiding this comment

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

I personally don't like this, this will increase variability.

What is your proposal? It just implements upstream feature design: With net and tools, there is projsync. But there is no AND in `vcpkg_check_features´.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use the original method:

if ("tools" IN_LIST FEATURES OR "net" IN_LIST FEATURES)
    set(ADDITIONAL_OPTIONS "-DBUILD_PROJSYNC=ON")
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original method was variation in number of FEATURES supplied to check_feature_options (before check_feature_options, sometimes adding tools BUILD_PROJSYNC to EXTRA_FEATURES).

You propose another alternative, adding the setting to another variable, independent of check_feature_options.

I dislike both approaches for the inconsistency. All other tools are handled by vcpkg_check_features. vcpkg_check_features doesn't only set FEATURE_OPTIONS (functional effect) but also individual variables (side effect). I wanted to have the same behaviour for BUILD_PROJSYNC.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #20147.
This situation cannot be handled in vcpkg_check_features at present, we can only do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This situation can be handled in vcpkg_check_features, but only in the proposed way: Changing the feature name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, please complete this PR and let my colleague review it further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will complete it when the pc file PR is merged (#20458). We need to carry transitive usage requirements for static linkage. That's why it is a draft.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewritten.

@JackBoosY JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label Oct 13, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 11, 2021

Depends on #20480.

@dg0yt dg0yt changed the title [proj4] Revise features and dependencies [proj4] Update to 8.1.1, revise features and dependencies Nov 19, 2021
@dg0yt dg0yt mentioned this pull request Nov 22, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 23, 2021

Seems to need #21594 which in turn needs #21599 which needs review from @JackBoosY.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 80be868eb552a8d2f94cda5ee1ce94d49612ce12 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index c557c01..674bfc4 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -5401,7 +5401,7 @@
       "port-version": 1
     },
     "proj4": {
-      "baseline": "8.0.0",
+      "baseline": "8.1.1",
       "port-version": 0
     },
     "prometheus-cpp": {

@dg0yt dg0yt marked this pull request as ready for review December 7, 2021 06:38
@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 7, 2021

libcrypto from openssl needs user32.lib. This is not an issue with CMake which has implicit link libraries. But it is an isse with nmake when relying only on pkg-config.

libcrypto.lib(cryptlib.obj) : error LNK2019: unresolved external symbol __imp_GetProcessWindowStation referenced in function OPENSSL_isservice
libcrypto.lib(cryptlib.obj) : error LNK2019: unresolved external symbol __imp_GetUserObjectInformationW referenced in function OPENSSL_isservice
libcrypto.lib(cryptlib.obj) : error LNK2019: unresolved external symbol __imp_MessageBoxW referenced in function OPENSSL_showfatal
spatialite.dll : fatal error LNK1120: 3 unresolved externals

Alternative A: Update libcrypto.pc. Rebuilds all dependencies of openssl.
Alternative B: Pass cmake's implicit link libraries to nmake. Rebuilds all dependencies of vcpkg_build_nmake.

I tend towards B. It seems to more consistent overall behaviour in vcpkg.

@Neumann-A
Copy link
Contributor

Neumann-A commented Dec 7, 2021

@dg0yt Put them into ENV{_LINK_} just like vcpkg_configure_make? (or ENV{LIBS} if nmake uses that...)

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 7, 2021

@dg0yt But them into ENV{_LINK_} just like vcpkg_configure_make? (or ENV{LIBS} if nmake uses that...)

Something like vcpkg_configure_make does, for B, yes.

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 7, 2021

Alternative A: Update libcrypto.pc. Rebuilds all dependencies of openssl.
Alternative B: Pass cmake's implicit link libraries to nmake. Rebuilds all dependencies of vcpkg_build_nmake.

To unblock the highly desired proj changes now, I implemented alternative C: Add user32.lib explicitly where needed.

In the long run, I still prefer alternative B. But this would be a fundamental change. It would better go to a new port vcpkg-nmake instead of the currrent unversioned maintainer script.

@dg0yt dg0yt mentioned this pull request Dec 7, 2021
1 task
@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 7, 2021

CI strangeness: On osx, it rebuilds some ports which should be cached and are not affected by the changes. From the logs:

Build 1, 0f07f0f: Rebuilding readosm,

Agent: vcpkg-eg-mac-03

HEAD is now at 65433e8c3 Merge 0f07f0fe0c231c9e8da14def0f28642caf46283b into 431d3810aa9241aed49b14367be417f225d2af5e


                          freexl:x64-osx:     pass: 3336081133f426c8aa88c625fd0efa8c6f03cd9d481226770fb0c0abfa94bb5f
                         readosm:x64-osx:     pass: bd8d2ef7219f20a0174d386abcd73fc717429c326efe2be276a9e7c83f75416b

2021-12-07T12:15:13.8180670Z Uploaded binaries to 1 HTTP remotes.
2021-12-07T12:15:13.8183800Z Installing package readosm[core]:x64-osx...
2021-12-07T12:15:13.8256250Z Elapsed time for package readosm:x64-osx: 31.13 s

Build 2, d5bc878: Rebuiding freexl

Agent: vcpkg-eg-mac-03

HEAD is now at b5978228c Merge d5bc878bd0c63f843139520a133519e3f0b0963a into 431d3810aa9241aed49b14367be417f225d2af5e

                          freexl:x64-osx:     pass: 3336081133f426c8aa88c625fd0efa8c6f03cd9d481226770fb0c0abfa94bb5f
                         readosm:x64-osx:     pass: bd8d2ef7219f20a0174d386abcd73fc717429c326efe2be276a9e7c83f75416b

2021-12-07T12:25:59.1985960Z Uploaded binaries to 1 HTTP remotes.
2021-12-07T12:25:59.1988430Z Installing package freexl[core]:x64-osx...
2021-12-07T12:25:59.2056200Z Elapsed time for package freexl:x64-osx: 41.47 s

Build 2 was started after completing build 1, the ABI hashes didn't change, and master didn't change. Neither freexl nor readosm depend on the changed ports.

@dg0yt dg0yt mentioned this pull request Dec 8, 2021
5 tasks
@JackBoosY JackBoosY removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Dec 9, 2021
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Dec 9, 2021
@BillyONeal BillyONeal merged commit a4f322c into microsoft:master Dec 10, 2021
@BillyONeal
Copy link
Member

Thanks for the updates!

@dg0yt dg0yt deleted the proj-features branch December 11, 2021 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[proj4] update to 8.1.1
4 participants