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

[krb5, curl] Add new port, and add new feature curl[gssapi] #38402

Merged
merged 2 commits into from May 6, 2024

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Apr 24, 2024

  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

krb5 have an issue of absolute paths that I am not sure how to fix, also I didn't succeeded to compile it on windows, but this PR is a good start.

warning: There should be no absolute paths, such as the following, in an installed package:
  /home/tal/vcpkg/packages/krb5_x64-linux
  /home/tal/vcpkg/installed
  /home/tal/vcpkg/buildtrees/krb5
  /home/tal/vcpkg/downloads
Absolute paths were found in the following files:
  /home/tal/vcpkg/packages/krb5_x64-linux/tools/krb5/bin/compile_et
  /home/tal/vcpkg/packages/krb5_x64-linux/tools/krb5/bin/krb5-config
  /home/tal/vcpkg/packages/krb5_x64-linux/tools/krb5/debug/bin/compile_et
  /home/tal/vcpkg/packages/krb5_x64-linux/tools/krb5/debug/bin/krb5-config
error: Found 1 post-build check problem(s). To submit these ports to curated catalogs, please first correct the portfile: /home/tal/vcpkg/ports/krb5/portfile.cmake

@talregev talregev changed the title [krb5, curll] Add new port, and add new feature curl[gssapi] [krb5, curl] Add new port, and add new feature curl[gssapi] Apr 24, 2024
@LilyWangLL LilyWangLL added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:new-port The issue is requesting a new library to be added; consider making a PR! labels Apr 25, 2024
ports/curl/dependencies.patch Outdated Show resolved Hide resolved
SOURCE_PATH "${SOURCE_PATH}/src"
AUTOCONFIG
OPTIONS
CFLAGS=-fcommon
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this override CFLAGS completely?

Copy link
Contributor Author

@talregev talregev Apr 26, 2024

Choose a reason for hiding this comment

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

I think not, How we can verify it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from build log:

/usr/bin/cc -DHAVE_CONFIG_H  -I../../include -I../.././../src/krb5-1-8a38cd677f.clean/src/include  -DKRB5_DEPRECATED=1 -DKRB5_PRIVATE -DNDEBUG -fcommon -Wall -Wcast-align -Wshadow -Wmissing-prototypes -Wno-format-zero-length -Woverflow -Wstrict-overflow -Wmissing-format-attribute -Wmissing-prototypes -Wreturn-type -Wmissing-braces -Wparentheses -Wswitch -Wunused-function -Wunused-label -Wunused-variable -Wunused-value -Wunknown-pragmas -Wsign-compare -Werror=uninitialized -Wno-maybe-uninitialized -Werror=pointer-arith -Werror=int-conversion -Werror=incompatible-pointer-types -Werror=discarded-qualifiers -Werror=implicit-int -Werror=declaration-after-statement -Werror-implicit-function-declaration -pthread  -c ../.././../src/krb5-1-8a38cd677f.clean/src/util/support/threads.c

Copy link
Contributor

Choose a reason for hiding this comment

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

So this really overrides essential flags from vcpkg, in particular -fPIC, -g/-O3.
Try

Suggested change
CFLAGS=-fcommon
"CFLAGS=-fcommon \$CFLAGS"

But why is the extra flag needed at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without the flag:

/usr/bin/ld: ../../lib/libkadm5srv_mit.a(server_kdb.o):/home/tal/vcpkg/buildtrees/krb5/x64-linux-dbg/lib/kadm5/srv/../../.././../src/krb5-1-8a38cd677f.clean/src/lib/kadm5/srv/server_kdb.c:18: multiple definition of `master_keyblock'; kdb5_util.o:/home/tal/vcpkg/buildtrees/krb5/x64-linux-dbg/kadmin/dbutil/../.././../src/krb5-1-8a38cd677f.clean/src/kadmin/dbutil/kdb5_util.c:109: first defined here
collect2: error: ld returned 1 exit status

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, really old (> 10 years) C code, and gcc 10 making '-fno-common` the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this really overrides essential flags from vcpkg, in particular -fPIC, -g/-O3. Try

Thank you for your suggestion, it help!

/usr/bin/cc -DHAVE_CONFIG_H  -I../../include -I../.././../src/krb5-1-8a38cd677f.clean/src/include -I../.././../src/krb5-1-8a38cd677f.clean/src/lib/gssapi/generic -I../.././../src/krb5-1-8a38cd677f.clean/src/lib/gssapi/krb5 -I../../lib/gssapi/generic -I../../lib/gssapi/krb5 -DKRB5_DEPRECATED=1 -DKRB5_PRIVATE  -fcommon -fPIC -g -Wall -Wcast-align -Wshadow -Wmissing-prototypes -Wno-format-zero-length -Woverflow -Wstrict-overflow -Wmissing-format-attribute -Wmissing-prototypes -Wreturn-type -Wmissing-braces -Wparentheses -Wswitch -Wunused-function -Wunused-label -Wunused-variable -Wunused-value -Wunknown-pragmas -Wsign-compare -Werror=uninitialized -Wno-maybe-uninitialized -Werror=pointer-arith -Werror=int-conversion -Werror=incompatible-pointer-types -Werror=discarded-qualifiers -Werror=implicit-int -Werror=declaration-after-statement -Werror-implicit-function-declaration -pthread  -c ../.././../src/krb5-1-8a38cd677f.clean/src/kadmin/server/auth.c

"description": "Network authentication protocol",
"homepage": "https://web.mit.edu/kerberos/",
"license": "MIT",
"supports": "!windows & !arm"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why shouldn't it support arm? Maybe CFLAGS issue?

Copy link
Contributor Author

@talregev talregev Apr 26, 2024

Choose a reason for hiding this comment

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

I am not sure, I didn't debug it. Also I didn't succeeded to compile on Windows.
As I see it, I can insert this port without the support of windows and arm, and others can take this port and improve it.

@talregev
Copy link
Contributor Author

@dg0yt How I can solve the absolute path errors?

@talregev talregev force-pushed the TalR/krb5 branch 2 times, most recently from 08c0f27 to af112d8 Compare April 26, 2024 18:31
@talregev talregev marked this pull request as ready for review April 26, 2024 18:31
@talregev
Copy link
Contributor Author

talregev commented Apr 26, 2024

Temporary workaround for absolute path problem, is to delete the tools that have the absolute path.

@talregev talregev force-pushed the TalR/krb5 branch 2 times, most recently from 72763fa to ae9b2c8 Compare April 28, 2024 03:20
@talregev
Copy link
Contributor Author

@LilyWangLL
PR is ready for review.
I am not sure why osx is failing.

@LilyWangLL
Copy link
Contributor

Feature curl[gssapi] passed on x64-linux.

ports/krb5/portfile.cmake Outdated Show resolved Hide resolved
@talregev
Copy link
Contributor Author

@LilyWangLL Ready for review.

@talregev
Copy link
Contributor Author

talregev commented May 1, 2024

@LilyWangLL I answer all @dg0yt request. Can you review again?

@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label May 6, 2024
@data-queue data-queue merged commit 20d55c6 into microsoft:master May 6, 2024
17 checks passed
@JonLiu1993
Copy link
Member

@talregev, this PR resulted in ci pilpline REGRESSION, but the strange thing is that I cannot reproduce these problems locally. Could you please take a look?

REGRESSION: libsoup:x64-linux failed with BUILD_FAILED. If expected, add libsoup:x64-linux=fail to /mnt/vss/_work/1/s/scripts/azure-pipelines/../ci.baseline.txt.
REGRESSION: lunarg-vulkantools:x64-linux failed with BUILD_FAILED. If expected, add lunarg-vulkantools:x64-linux=fail to /mnt/vss/_work/1/s/scripts/azure-pipelines/../ci.baseline.txt.
REGRESSION: qt5-serialbus:x64-linux failed with BUILD_FAILED. If expected, add qt5-serialbus:x64-linux=fail to /mnt/vss/_work/1/s/scripts/azure-pipelines/../ci.baseline.txt.

libsoup:x64-linux:
package-x64-linux-dbg-out.log
lunarg-vulkantools:x64-linux:
install-x64-linux-dbg-out.log
qt5-serialbus:x64-linux:
package-build-x64-linux-dbg-err.log

@talregev
Copy link
Contributor Author

talregev commented May 8, 2024

I think what happened is that krb5 port is connected automatic to these ports. by find_packeges.
It not detected on my PR because I didn't change the failed ports, and ci take them by hash.

When the failed ports changed, they detected the krb5 port, and try to compile it.
But the port needed detected with pkg-config as i did in curl.

We have 4 options:

  • revert this PR.
  • fix krb5 port that it will detected correctly also with find_package
  • fix other ports that they detect krb5 with pkg-config
  • disable the automatic detection of krb5 in the other ports.

@talregev
Copy link
Contributor Author

talregev commented May 8, 2024

@dg0yt I will glad your expert opinion as well 🙏🏻

@dg0yt
Copy link
Contributor

dg0yt commented May 8, 2024

There are two problems:

  1. installation order problem. Some port auto-detect the presence of krb and use it. Then the binary artifacts depend on krb binaries without declaring them.
    This must be solved downstream. Disabling krb5 support might be the easiest fix: Not a regression within the vcpkg ecosystem. But maybe a problem for users who relied on system libs being used instead.
  2. usage requirements problem. I don't know if we have a CI configuration to see that the pkg-config files are okay.
    What could be done immediately (and should have been part of the PR) is to request the krb5 feature for linux in test port vcpkg-ci-curl. This would build the curl executable with krb5.
    For CMake, there is no official Find module, so there is nothing to wrap. Downstream users need to be patched to use pkg-config from cmake to pick the full set of link libraries for static linkage. And they have to pass it on in their exports.
  • revert this PR.

😵 💥

  • fix krb5 port that it will detected correctly also with find_package

No, krb5 has no single item to fix or wrap for CMake.

  • fix other ports that they detect krb5 with pkg-config

Problem 2 solved right. But mind problem 1.

  • disable the automatic detection of krb5 in the other ports.

Problem 2 eliminated. Problem 1 solved the easy way.

@dg0yt
Copy link
Contributor

dg0yt commented May 8, 2024

As a quick CI workaround, krb5 might be added as skip to ci.baseline.txt. Unfortunately, this will not reliably unblock other PRs - they would still pull fail due to the broken binary artifacts in the cache.
And it would make the modified test port fail. So that the test port modifcation would need to come after the baseline change, and revert it.

yurybura pushed a commit to yurybura/vcpkg that referenced this pull request May 8, 2024
…t#38402)

- [x] Changes comply with the [maintainer
guide](https://github.com/microsoft/vcpkg-docs/blob/main/vcpkg/contributing/maintainer-guide.md).
- [x] The name of the port matches an existing name for this component
on https://repology.org/ if possible, and/or is strongly associated with
that component on search engines.
- [x] Optional dependencies are resolved in exactly one way. For
example, if the component is built with CMake, all `find_package` calls
are REQUIRED, are satisfied by `vcpkg.json`'s declared dependencies, or
disabled with
[CMAKE_DISABLE_FIND_PACKAGE_Xxx](https://cmake.org/cmake/help/latest/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html).
- [x] The versioning scheme in `vcpkg.json` matches what upstream says.
- [x] The license declaration in `vcpkg.json` matches what upstream
says.
- [x] The installed as the "copyright" file matches what upstream says.
- [x] The source code of the component installed comes from an
authoritative source.
- [x] The generated "usage text" is accurate. See
[adding-usage](https://github.com/microsoft/vcpkg-docs/blob/main/vcpkg/examples/adding-usage.md)
for context.
- [x] The version database is fixed by rerunning `./vcpkg x-add-version
--all` and committing the result.
- [x] Only one version is in the new port's versions file.
- [x] Only one version is added to each modified port's versions file.

krb5 have an issue of absolute paths that I am not sure how to fix, also
I didn't succeeded to compile it on windows, but this PR is a good
start.

```
warning: There should be no absolute paths, such as the following, in an installed package:
  /home/tal/vcpkg/packages/krb5_x64-linux
  /home/tal/vcpkg/installed
  /home/tal/vcpkg/buildtrees/krb5
  /home/tal/vcpkg/downloads
Absolute paths were found in the following files:
  /home/tal/vcpkg/packages/krb5_x64-linux/tools/krb5/bin/compile_et
  /home/tal/vcpkg/packages/krb5_x64-linux/tools/krb5/bin/krb5-config
  /home/tal/vcpkg/packages/krb5_x64-linux/tools/krb5/debug/bin/compile_et
  /home/tal/vcpkg/packages/krb5_x64-linux/tools/krb5/debug/bin/krb5-config
error: Found 1 post-build check problem(s). To submit these ports to curated catalogs, please first correct the portfile: /home/tal/vcpkg/ports/krb5/portfile.cmake
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! 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.

None yet

5 participants