Skip to content

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

Merged
BillyONeal merged 29 commits intomicrosoft:masterfrom
wrobelda:gnutls_windows
Sep 2, 2022
Merged

[shiftmedia-gnutls] new port (Windows fork of GnuTLS)#18029
BillyONeal merged 29 commits intomicrosoft:masterfrom
wrobelda:gnutls_windows

Conversation

@wrobelda
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@NancyLi1013
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@NancyLi1013
Copy link
Copy Markdown
Contributor

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

@wrobelda
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

wrobelda commented Oct 6, 2021

@NancyLi1013 this is now ready for a review

@wrobelda
Copy link
Copy Markdown
Contributor Author

Also see #26364, which was merged in yesterday

@BillyONeal
Copy link
Copy Markdown
Member

Also see #26364, which was merged in yesterday

That doesn't try to fetch from multiple repos though.

@BillyONeal
Copy link
Copy Markdown
Member

To clarify, @JavierMatosD is asking about that we're downloading sources on line 8 and line 21 in portfile.cmake which makes what we are actually packaging here unclear (at least, to the two of us). The comment isn't about the use of the shiftmedia fork(s) in the first place which you have already addressed by changing the port's name.

@wrobelda
Copy link
Copy Markdown
Contributor Author

Oh, the other file is gnulib, not gnutls, and is a requirement for compilation of the latter.

@wrobelda
Copy link
Copy Markdown
Contributor Author

@JavierMatosD is that response satisfactory?

@BillyONeal
Copy link
Copy Markdown
Member

Oh, the other file is gnulib, not gnutls, and is a requirement for compilation of the latter.

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

@wrobelda
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Co-authored-by: Billy O'Neal <bion@microsoft.com>
Copy link
Copy Markdown

@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
Copy Markdown

@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
Copy Markdown
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
Copy link
Copy Markdown
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.

9 participants