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

[shiftmedia-gnutls] new port (Windows fork of GnuTLS) #18029

Merged
merged 29 commits into from
Sep 2, 2022

Conversation

wrobelda
Copy link
Contributor

@wrobelda wrobelda commented May 20, 2021

Describe the pull request
Uses SMP fork of GnuTLS to add a new fork, based on the suggestion from @BillyONeal (#18029 (comment))

@dg0yt
Copy link
Contributor

dg0yt commented May 20, 2021

I would prefer when one port would use only one particular set of sources and patches, so that I can build for different target triplets without worrying about the state of the port's sources in the buildtrees folder.
(I know it is only a draft at this point.)

@wrobelda
Copy link
Contributor Author

I'm sorry, I am not sure what you expect here. All of the portfiles which use ShiftMediaProject forks to provide Windows support behave like this.

@dg0yt
Copy link
Contributor

dg0yt commented May 20, 2021

I know that a lot of ports do different patching for different triplet, but this is the first time I note different sources.

  • What is going to happen when I want to cross-build for Android from the same vcpkg root?
  • If I need to collect the sources, am I expected to collect it separately for each triplet (read: vcpkg --download-only)?

@wrobelda
Copy link
Contributor Author

wrobelda commented May 20, 2021

I know that a lot of ports do different patching for different triplet, but this is the first time I note different sources.

See e.g. gmp, nettle for comparison.

  • What is going to happen when I want to cross-build for Android from the same vcpkg root?

I am not sure I follow, again. Sources are downloaded into separate folders with unique names, usually consisting of a version/revision number and a hash sum. Since sources for Windows and *nix are different in those portfiles, they get downloaded into separate folders and patches are applied respectively. This behavior is no different from building a different (older) port version on demand.

@dg0yt
Copy link
Contributor

dg0yt commented May 20, 2021

Sources are downloaded into separate folders with unique names, usually consisting of a version/revision number and a hash sum.

I see, and you refering to unpacking indeed. So I only need to worry about some patches which are not applied uniformly.

Anyway, I find it confusing to have different sources. I admit I'm biased, with experience in building RPMS and DEBs.

@wrobelda
Copy link
Contributor Author

wrobelda commented May 20, 2021

So I only need to worry about some patches which are not applied uniformly.

I don't think there's anything to worry at all. Those Windows-specific patches will only be applied on top of ShiftMediaProject source code, which, in turn, is only used for Windows build.

Anyway, I find it confusing to have different sources. I admit I'm biased, with experience in building RPMS and DEBs.

These are definitely exceptions. ShiftMediaProject has whole bunch of MSVC-compatible forks, which are very convenient to use. I even discussed a possibility of pushing those changes upstream, but as you can imagine, it's a voluntary work on all fronts:

ShiftMediaProject/gnutls#13 (comment)

@NancyLi1013 NancyLi1013 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label May 21, 2021
@wrobelda wrobelda force-pushed the gnutls_windows branch 2 times, most recently from efa69b7 to 31e8d62 Compare May 25, 2021 20:57
ports/libgnutls/portfile.cmake Outdated Show resolved Hide resolved
ports/libgnutls/portfile.cmake Outdated Show resolved Hide resolved
ports/libgnutls/pkgconfig.patch Outdated Show resolved Hide resolved
ports/libgnutls/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

-- Downloading https://www.gnupg.org/ftp/gcrypt/gnutls/v3.{PACKAGE_VERSION_MINOR}/gnutls-3.6.15.tar.xz -> gnutls-3.6.15.tar.xz...
-- Downloading https://www.gnupg.org/ftp/gcrypt/gnutls/v3.{PACKAGE_VERSION_MINOR}/gnutls-3.6.15.tar.xz... Failed. Status: 22;"HTTP response code said error"
CMake Error at scripts/cmake/vcpkg_download_distfile.cmake:184 (message):
      
      Failed to download file.
      If you use a proxy, please check your proxy setting. Possible causes are:

{PACKAGE_VERSION_MINOR} should be ${{PACKAGE_VERSION_MINOR}}.

I have a question here, why do you use different repo in different platforms? Also I noticed there was a new version 3.16.6. Why do you not use the latest?

@NancyLi1013
Copy link
Contributor

Please ping me if this is ready for review. @wrobelda

@NancyLi1013
Copy link
Contributor

Hi @wrobelda Is work still being done for this PR? Seems there is no any progress for a long time.

@wrobelda
Copy link
Contributor Author

Hi @wrobelda Is work still being done for this PR? Seems there is no any progress for a long time.

I am going to return to it as soon as all of my kf5* ports are merged.

@wrobelda wrobelda marked this pull request as ready for review October 6, 2021 21:52
@wrobelda
Copy link
Contributor Author

wrobelda commented Oct 6, 2021

@NancyLi1013 this is now ready for a review

@wrobelda
Copy link
Contributor Author

wrobelda commented Aug 29, 2022

It seems like that should be a separate port? (It looks like an independent thing...)

It actually isn't. The GnuTLS relies on bunch of code from Gnulib which is directly referenced by SMP VS project files, see: https://github.com/ShiftMediaProject/gnutls/blob/master/SMP/libgnutls_files.props

They don't ship that code, though — they provide a script that populates the gnulib/ subfolder:
https://github.com/ShiftMediaProject/gnutls/blob/master/bootstrap
Also see: https://github.com/ShiftMediaProject/gnutls/blob/master/SMP/readme.txt

What I am merely doing here is exactly that: populating that folder with the code it expects. The original gnulib/ subfolder is a git submodule, which is why I am extracting the gnulib source into it.

Note that this is not SMP's doing — the upstream GnuTLS is like that: https://gitlab.com/gnutls/gnutls/-/blob/master/.gitmodules

Obviously this still doesn't mean gnulib couldn't be built as a library independently, but that's way more complex and basically against the grain of what GnuTLS does itself.

EDIT: but now that I am reading the gnulib's own docs, they say:

Gnulib takes a different approach. Its components are intended to be shared at the source level, rather than being a library that gets built, installed, and linked against. Thus, there is no distribution tarball; the idea is to copy files from Gnulib into your own source tree.

So this is all by design, gnulib is not an actual lib.

BillyONeal
BillyONeal previously approved these changes Aug 29, 2022
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

This approval is contingent on:

  • Another vcpkg maintainer signing off on the multiple-upstreams thing
  • The 'supports' expression being fixed.
  • The additional quotes in file(RENAME added.

All other review comments are nitpicks.

Thanks for your contribution!

RELEASE_CONFIGURATION ${CONFIGURATION_RELEASE}
DEBUG_CONFIGURATION ${CONFIGURATION_DEBUG}
SKIP_CLEAN
OPTIONS /p:YasmPath="${YASM}" /p:OutDir=..\\msvc
Copy link
Member

Choose a reason for hiding this comment

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

I'm very sad to see another thing depending on yasm given how full of flaky memory corruption bugs it has turned out to be :(. No change requested.

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 agree. Eventually I hope we can get a vanilla version of this to compile with clang-cl, similarly to the ongoing #26424.

ports/shiftmedia-libgnutls/vcpkg.json Outdated Show resolved Hide resolved
ports/shiftmedia-libgnutls/vcpkg.json Outdated Show resolved Hide resolved
ports/shiftmedia-libgnutls/portfile.cmake Outdated Show resolved Hide resolved
Co-authored-by: Billy O'Neal <bion@microsoft.com>
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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for shiftmedia-libgnutls have changed but the version was not updated
version: 3.7.6
old SHA: 1de8a6097b77f987022f00eeeebacb163a4820ee
new SHA: 35797029a653421f3d04186ef00bc126b4d3ada1
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

Co-authored-by: Billy O'Neal <bion@microsoft.com>
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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for shiftmedia-libgnutls have changed but the version was not updated
version: 3.7.6
old SHA: 1de8a6097b77f987022f00eeeebacb163a4820ee
new SHA: 2151242268c29f3642323e0a97034a4a9c52fa09
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

github-actions[bot]
github-actions bot previously approved these changes Aug 29, 2022
github-actions[bot]
github-actions bot previously approved these changes Aug 29, 2022
github-actions[bot]
github-actions bot previously approved these changes Aug 29, 2022
github-actions[bot]
github-actions bot previously approved these changes Aug 29, 2022
github-actions[bot]
github-actions bot previously approved these changes Aug 29, 2022
@wrobelda
Copy link
Contributor Author

This approval is contingent on:

  • Another vcpkg maintainer signing off on the multiple-upstreams thing

To be fair, this is only required because vcpkg_from_github() is not currently able to download from git modules.

@BillyONeal BillyONeal merged commit 7079273 into microsoft:master Sep 2, 2022
@BillyONeal
Copy link
Member

Thanks!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants