[zstd] No debug postfix. No renaming of CMake config files. #22822

dg0yt · 2022-01-27T18:10:29Z

What does your PR fix?

Restore upstream's original filenames.

Downstreams are unaware of vcpkg's debug postfix. That's why there are some patches.

Unfortunately, downstreams are also unaware of zstd's _static postfix which is only used on Windows, when building with CMake. This PR remove that postfix, too.
Which triplets are supported/not supported? Have you updated the CI baseline?

unchanged, no
Does your PR follow the maintainer guide?

Yes. The port didn't.
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes.

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

ports/blosc/vcpkg.json
ports/boost-iostreams/vcpkg.json
ports/elfutils/vcpkg.json
ports/libwandio/vcpkg.json
ports/qt5-base/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

dg0yt · 2022-01-28T07:14:55Z

We should really consider patching out the _static postfix from the windows builds. It is not used on other platforms because it only serves to avoid a clash between import lib and static lib name when building both (which vcpkg doesn't). Downstreams generally always look for the zstd lib name, but they don't look for zstd_static, and even in vcpkg patching turned out to be incomplete.

dg0yt · 2022-01-28T07:16:56Z

PS this is a drive-by from testing gdal's coming cmake build system which only looks for zstd in FindZSTD.cmake.

dg0yt · 2022-01-28T07:25:34Z

We should really consider patching out the _static postfix from the windows builds.

This would probably also reduce the amount of patching from individual Find modules to zstd config. Where zstd config suffers from using different target names for static vs. shared.
Example: #22834

Neumann-A · 2022-01-28T08:04:24Z

We should really consider patching out the _static postfix from the windows builds.

No. We are not in charge of upstream naming. Add a wrapper which does a find_library call before the find_package call.
If downstream doesn't correctly abide to upstream naming we have to fix downstream usage.

using different target names for static vs. shared.

add an interface target (not ALIAS!) without the suffix which links to one of those depending on library linkage. You can probably use a genex which tests for the existence of one of the targets and use the other otherwise.,

dg0yt · 2022-01-28T12:59:20Z

My proposal to remove _static aims at two things:

Reduce patching.
Avoid unexpect build results (not using zstd or using system zstd).

We should really consider patching out the _static postfix from the windows builds.

No. We are not in charge of upstream naming. .... If downstream doesn't correctly abide to upstream naming we have to fix downstream usage.

I fully support the general policy.
However, zstd seems to offer different build systems, and AFAICS only the cmake one adds the _static postfix (and only for windows). So downstreams are not so wrong.
In addition, updating the existing zstd patches left the impression that vcpkg contributors are unaware of the issue.

blosc, libwandio and qt5-base didn't have it.
boost might need it, but I don't know the build system. EDIT: Note [boost-iostreams] Fail to build as x64-windows-static in Debug Mode #22817
elfutils had it but doesn't support windows.

Add a wrapper which does a find_library call before the find_package call.

There is no official CMake module so it would be a wrapper for what?
ZSTD_LIBRARY_RELEASE or Zstd_LIBRARY?

The wrapper also doesn't help with autotools.

using different target names for static vs. shared.

add an interface target (not ALIAS!) without the suffix which links to one of those depending on library linkage. You can probably use a genex which tests for the existence of one of the targets and use the other otherwise.

I have seen the genex, and it is fine.
What is the problem with the ALIAS? I use them in the cmake project include of the gdal-cmake PR. It turns out that CMake puts the imported target in the generated gdal config instead of the alias. This is nice, because the gdal config doesn't need to provide the alias again.
(I do see the problems with using them in a wrapper when facing CMake 3.4.)

Neumann-A · 2022-01-28T13:37:57Z

However, zstd seems to offer different build systems, and AFAICS

donwstream as to put up with every possibility upstream supports. If they don't they are wrong. But there are reason to have different naming... simply people want to have all variants in one and the same directory which is only possible if the names are different.

qt5-base didn't have it.

I am aware of the issues but as far as I remembered somebody renamed the variant to the non static variant. So I didn't bother. Wasn't I the one who left the comment about CMAKE_DEBUG_SUFFIX breaking maintainer guides? I just didn't change that because I didn't want to fix the fallout of that change ;)
(But qt5 could also use pkg-config)

elfutils had it but doesn't support windows.

Wondering if that support statement should really be !MSVC instead. (Did I add that support statement?)

The wrapper also doesn't help with autotools.

Downstream should use pkg-config !? (Instead of doing a find_library call the autotools way. )

There is no official CMake module so it would be a wrapper for what?

Ok then no wrapper just manually fixing every FindZSTD|Findzstd or providing our own version. I wish vcpkg would support vcpkg based modules instead of the wrapper. The main reason this isn't done because ras0219 says that some ports bomb CMAKE_MODULE_PATH by set(CMAKE_MODULE_PATH "myfunnypath").

What is the problem with the ALIAS?

Doesn't have the same semantics as an IMPORTED target (for example you cannot promote an ALIAS to global). If you Wrap an IMPORTED targets always use an INTERFACE IMPORTED target und link the one you want to wrap (This is basically the Qt6 way). This way you can wrap different targets like e.g. a PkgConfig::ZSTD target.

dg0yt · 2022-01-28T14:02:02Z

Wasn't I the one who left the comment about CMAKE_DEBUG_SUFFIX breaking maintainer guides?

Yes. You also wrote:

# Removing it probably requires a vcpkg-cmake-wrapper.cmake to correct downstreams FindZSTD.cmake.

But actually the unexpected debug postfix created the the need for a wrapper. As does the _import postfix.

Downstream should use pkg-config !?

That's how I would patch. Generally, downstreams using autotools are really ambitiuous when it comes to MSVC support.

Sidenote: Even Find modules from CMake use pkg-config to get a hint for the prefix. But instead of using all information, they do their own lookup with a limited set of names...

Summary on `_import` postfix

Current state: We patch the world and don't tell downstreams.
Proposal A: We tell downstreams that they do it wrong, and in the meantime we continue to patch the world.
Proposal B: We patch the postfix out of zstd.

JackBoosY · 2022-01-28T17:42:10Z

I think we should follow @Neumann-A suggestion since we have the policy that we shouldn't change the suffix which the upstream made.

dg0yt · 2022-01-28T21:09:10Z

@ras0219-msft What is your opinion on removing _static as an exception?

I could put that to another PR, but when it is merged separately, it will force another major rebuild on users.

dg0yt · 2022-01-29T06:03:39Z

FTR: The current state is ready for review, aligning the port with maintainer guidelines.

PhoebeHui · 2022-01-29T08:56:25Z

I vote for removing _static, I don't like to add these patches for handling the case, according to the policy, this should be accepted in vcpkg.

I noticed it also cause issue #22817.

ports/boost-iostreams/b2-options.cmake

dg0yt · 2022-01-29T09:44:23Z

according to the policy, this should be accepted in vcpkg.

In particular:

Static and shared variants often should be renamed to a common scheme. This enables consumers to use a common name and be ignorant of the downstream linkage. This is safe because we only make one at a time available.

ras0219-msft · 2022-02-01T20:18:19Z

To summarize the current affairs as I see it:

The most important part of the relevant policy is ... we should not introduce a new name.. Specifically, we should NOT be introducing something outside the set of names that upstream uses (such as adding debug suffixes that don't exist otherwise).
Upstream provides multiple buildsystems with inconsistent naming. According to @dg0yt, the only way to get zstd_static is by using the CMake buildsystem on Windows -- other platforms and other buildsystems would have produced zstd for the same binary.
Upstream provides .pc files that always say -lzstd, even on Windows+static.
A vast number of consumers assume the library is named zstd

Given all that, I see renaming the binary from zstd_static to zstd as being the correct approach. I do not view this as a policy violation because upstream itself is inconsistent and zstd is definitely one of the names already in use. We are therefore not introducing any new names by this renaming and we are making the ecosystem much more uniform and reducing the amount of patching required in dependencies.

Because downstream libraries SHOULD support shared linking of zstd, this should not create any new problems or additional patching requirements -- it's a strict win.

Edit: The above is only addressing zlib_static -> zlib and doesn't talk about the debug suffix.

Obviously the debug suffix is against policy, since upstream doesn't ever produce zstdd without our meddling. Is anyone aware of which downstream FindZSTD.cmake files are referenced by

# Removing it probably requires a vcpkg-cmake-wrapper.cmake to correct downstreams FindZSTD.cmake

?

ras0219-msft · 2022-02-01T20:23:25Z

Finally, one other drive-by comment: in addition to the binary names, we definitely need an INTERFACE target like @Neumann-A posted in #22822 (comment) -- the current schism in cmake target names is a nightmare for correctness in dependents.

Neumann-A · 2022-02-01T20:35:55Z

Upstream provides multiple buildsystems with inconsistent naming.

Naming is not inconsistent. It is just taking into account that you cannot have a import library and a static library in the same directory since both would be called zstd.lib. That is why a suffix like _static is required. On every other platform this is not a problem since the library suffix is different libzstd.(a|so).
About pc files not considering _static: Similar reason: Nobody creates pc files on windows except for vcpkg.

dg0yt · 2022-02-02T18:53:23Z

Finally, one other drive-by comment: in addition to the binary names, we definitely need an INTERFACE target like @Neumann-A posted in #22822 (comment)

This would be an unofficial::libzstd. I don't want to offer this as recommended usage.

Inventing a new unofficial target can be avoided @Neumann-A 's other proposal
$<IF:$<TARGET_EXISTS:zstd::libzstd_shared>,zstd::libzstd_shared,zstd::libzstd_static>
as already used by ports ktx, libarchive. This would need to go to the usage file.

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

ports/blosc/vcpkg.json
ports/boost-iostreams/vcpkg.json
ports/elfutils/vcpkg.json
ports/libwandio/vcpkg.json
ports/qt5-base/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

ports/blosc/vcpkg.json
ports/boost-iostreams/vcpkg.json
ports/elfutils/vcpkg.json
ports/libwandio/vcpkg.json
ports/qt5-base/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

JackBoosY · 2022-02-05T08:05:52Z

I personally support this modification.

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/blosc/vcpkg.json
ports/boost-iostreams/vcpkg.json
ports/elfutils/vcpkg.json
ports/libwandio/vcpkg.json
ports/qt5-base/vcpkg.json

Valid values for the license field can be found in the documentation

dg0yt · 2022-02-11T08:03:02Z

Merge conflict resolved. Time for vcpkg team to move.

JackBoosY · 2022-02-11T08:04:05Z

Ping @ras0219-msft

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/blosc/vcpkg.json
ports/boost-iostreams/vcpkg.json
ports/elfutils/vcpkg.json
ports/libwandio/vcpkg.json
ports/qt5-base/vcpkg.json

Valid values for the license field can be found in the documentation

PhoebeHui

LGTM, double confirmed, it also fix the issue in #22817, thank you @dg0yt !

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/blosc/vcpkg.json
ports/boost-iostreams/vcpkg.json
ports/elfutils/vcpkg.json
ports/libwandio/vcpkg.json
ports/qt5-base/vcpkg.json

Valid values for the license field can be found in the documentation

ras0219-msft · 2022-02-15T00:36:42Z

After inspecting all references to zstd in ports/, I see ~8 cases where our downstream patches would be simplified by having an INTERFACE alias for zstd_shared to zstd_static (and vice versa). While I don't think there's much value in rev-ing all those ports now to take advantage, I find this to be sufficient evidence that the alias is a good idea and will make our collective lives easier in the future.

I've pushed an additional commit that does two things:

Minor fix to the .pc file to include -pthread if CMake lists it. This removes the need for the qt-base patch entirely (yay!) and is generally a good correctness fix.
Add the flavor-alias. Note that I have not removed the usage file, so we will still recommend to users the more complicated expression which will work correctly outside vcpkg.

github-actions

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/blosc/vcpkg.json
ports/boost-iostreams/vcpkg.json
ports/elfutils/vcpkg.json
ports/libwandio/vcpkg.json
ports/qt5-base/vcpkg.json

Valid values for the license field can be found in the documentation

ras0219-msft

LGTM if CI passes

JonLiu1993 assigned JackBoosY Jan 28, 2022

JackBoosY added the category:port-bug The issue is with a library, which is something the port should already support label Jan 28, 2022

dg0yt added 5 commits January 28, 2022 07:36

Use upstream's standard filenames

d99a96e

Update license info

a672d8f

CMake style

34e99cd

Revise downstream zstd patching

004a085

Update versions

433eac0

dg0yt force-pushed the zstd branch from 7956fc5 to 433eac0 Compare January 28, 2022 06:54

github-actions bot approved these changes Jan 28, 2022

View reviewed changes

dg0yt marked this pull request as ready for review January 28, 2022 07:11

PhoebeHui reviewed Jan 29, 2022

View reviewed changes

ports/boost-iostreams/b2-options.cmake Show resolved Hide resolved

dg0yt mentioned this pull request Jan 29, 2022

[boost-iostreams] Fail to build as x64-windows-static in Debug Mode #22817

Closed

PhoebeHui requested a review from ras0219-msft January 29, 2022 09:37

github-actions bot approved these changes Feb 3, 2022

View reviewed changes

dg0yt added 4 commits February 3, 2022 09:33

Don't use '_static' lib name suffix

e799593

Revise downstream zstd patching

e70364b

Add usage

e4e944c

Update versions

59cd441

dg0yt force-pushed the zstd branch from bf9f1f6 to 59cd441 Compare February 3, 2022 08:33

github-actions bot approved these changes Feb 3, 2022

View reviewed changes

strega-nil-ms added the requires:discussion label Feb 4, 2022

dg0yt added 2 commits February 11, 2022 08:58

Merge remote-tracking branch 'origin/master' into zstd

12682e7

Update versions

ca2ab38

github-actions bot approved these changes Feb 11, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into zstd

2fe78f4

github-actions bot approved these changes Feb 13, 2022

View reviewed changes

PhoebeHui removed the requires:discussion label Feb 14, 2022

PhoebeHui approved these changes Feb 14, 2022

View reviewed changes

PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Feb 14, 2022

[zstd] Simplify consumers via fixed .pc and linkage alias

e6d53f8

github-actions bot approved these changes Feb 15, 2022

View reviewed changes

[zstd] Fix missing IMPORTED

38fd5f9

github-actions bot approved these changes Feb 15, 2022

View reviewed changes

ras0219-msft approved these changes Feb 15, 2022

View reviewed changes

ras0219-msft merged commit d878305 into microsoft:master Feb 16, 2022

dg0yt deleted the zstd branch February 16, 2022 09:02

JackBoosY mentioned this pull request Mar 18, 2022

[proxygen]: fix dependency #22834

Merged

AbdulAbCellera mentioned this pull request Dec 24, 2022

[tiff] zstd 1.5.2#1 tiff feature config file name update #28523

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[zstd] No debug postfix. No renaming of CMake config files. #22822

[zstd] No debug postfix. No renaming of CMake config files. #22822

dg0yt commented Jan 27, 2022 •

edited

Loading

github-actions bot left a comment

dg0yt commented Jan 28, 2022 •

edited

Loading

dg0yt commented Jan 28, 2022

dg0yt commented Jan 28, 2022

Neumann-A commented Jan 28, 2022 •

edited

Loading

dg0yt commented Jan 28, 2022 •

edited

Loading

Neumann-A commented Jan 28, 2022

dg0yt commented Jan 28, 2022

JackBoosY commented Jan 28, 2022

dg0yt commented Jan 28, 2022 •

edited

Loading

dg0yt commented Jan 29, 2022

PhoebeHui commented Jan 29, 2022

dg0yt commented Jan 29, 2022

ras0219-msft commented Feb 1, 2022 •

edited

Loading

ras0219-msft commented Feb 1, 2022

Neumann-A commented Feb 1, 2022

dg0yt commented Feb 2, 2022

github-actions bot left a comment

github-actions bot left a comment

JackBoosY commented Feb 5, 2022

github-actions bot left a comment

dg0yt commented Feb 11, 2022

JackBoosY commented Feb 11, 2022

github-actions bot left a comment

PhoebeHui left a comment

github-actions bot left a comment

ras0219-msft commented Feb 15, 2022 •

edited

Loading

github-actions bot left a comment

ras0219-msft left a comment

[zstd] No debug postfix. No renaming of CMake config files. #22822

[zstd] No debug postfix. No renaming of CMake config files. #22822

Conversation

dg0yt commented Jan 27, 2022 • edited Loading

What does your PR fix?

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

Does your PR follow the maintainer guide?

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

github-actions bot left a comment

Choose a reason for hiding this comment

dg0yt commented Jan 28, 2022 • edited Loading

dg0yt commented Jan 28, 2022

dg0yt commented Jan 28, 2022

Neumann-A commented Jan 28, 2022 • edited Loading

dg0yt commented Jan 28, 2022 • edited Loading

Neumann-A commented Jan 28, 2022

dg0yt commented Jan 28, 2022

Summary on _import postfix

JackBoosY commented Jan 28, 2022

dg0yt commented Jan 28, 2022 • edited Loading

dg0yt commented Jan 29, 2022

PhoebeHui commented Jan 29, 2022

dg0yt commented Jan 29, 2022

ras0219-msft commented Feb 1, 2022 • edited Loading

ras0219-msft commented Feb 1, 2022

Neumann-A commented Feb 1, 2022

dg0yt commented Feb 2, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

JackBoosY commented Feb 5, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

dg0yt commented Feb 11, 2022

JackBoosY commented Feb 11, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

PhoebeHui left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

ras0219-msft commented Feb 15, 2022 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

ras0219-msft left a comment

Choose a reason for hiding this comment

dg0yt commented Jan 27, 2022 •

edited

Loading

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

dg0yt commented Jan 28, 2022 •

edited

Loading

Neumann-A commented Jan 28, 2022 •

edited

Loading

dg0yt commented Jan 28, 2022 •

edited

Loading

Summary on `_import` postfix

dg0yt commented Jan 28, 2022 •

edited

Loading

ras0219-msft commented Feb 1, 2022 •

edited

Loading

ras0219-msft commented Feb 15, 2022 •

edited

Loading