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

curl: add hyper support #357552

Closed
wants to merge 3 commits into from
Closed

Conversation

getchoo
Copy link
Member

@getchoo getchoo commented Nov 20, 2024

Closes #357409

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@getchoo getchoo added the backport release-24.11 Backport PR automatically label Nov 20, 2024
@JohnRTitor
Copy link
Contributor

This should target staging.

@getchoo getchoo marked this pull request as draft November 20, 2024 12:47
@github-actions github-actions bot added 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: haskell 6.topic: qt/kde 8.has: documentation This PR adds or changes documentation 8.has: changelog 6.topic: emacs Text editor 6.topic: rust 6.topic: golang 6.topic: stdenv Standard environment 6.topic: nodejs 6.topic: systemd 6.topic: java Including JDK, tooling, other languages, other VMs 6.topic: lib The Nixpkgs function library 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 6.topic: tcl labels Nov 20, 2024
@getchoo getchoo changed the base branch from master to staging November 20, 2024 12:51
@getchoo getchoo added backport staging-24.11 Backport PR automatically and removed backport release-24.11 Backport PR automatically labels Nov 20, 2024
@github-actions github-actions bot removed 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: haskell 6.topic: qt/kde 8.has: documentation This PR adds or changes documentation 8.has: changelog 6.topic: emacs Text editor labels Nov 20, 2024
@github-actions github-actions bot added backport release-24.11 Backport PR automatically and removed 6.topic: java Including JDK, tooling, other languages, other VMs 6.topic: lib The Nixpkgs function library 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 6.topic: tcl backport staging-24.11 Backport PR automatically labels Nov 20, 2024
@getchoo getchoo marked this pull request as ready for review November 20, 2024 12:53
Copy link
Contributor

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

since it's actually false, we are not shipping curl with hyper, unlike what the post wants.

I would let the issue stay open and change the issue title to "curl: move to the hyper backend".
I think it's great to add the support by default, once it is stable, that is.

@JohnRTitor JohnRTitor requested a review from vcunat November 20, 2024 12:54
pkgs/by-name/li/libhyper/package.nix Outdated Show resolved Hide resolved
#
# Make sure rustc always emits cdylib artifacts
postPatch = ''
ln -sf ${./Cargo.lock} Cargo.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

Upstream does not commit their lockfiles?

I think a better approach would be to ask upstream to commit their lockfile, mentioning our reproducibility requirements which needs the lockfile to compare the hash with.

Copy link
Contributor

@pluiedev pluiedev Nov 20, 2024

Choose a reason for hiding this comment

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

Hyper is usually used as a library, and Cargo's commit guidelines generally advise against including a Cargo.lock for libraries

Apparently that's a historical practice: https://doc.rust-lang.org/cargo/faq.html#why-have-cargolock-in-version-control leaves the choice open while stating that Cargo.lock doesn't affect library consumers. I guess it's okay to ask them to add a Cargo.lock then

Copy link
Member Author

Choose a reason for hiding this comment

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

@getchoo getchoo requested a review from milibopp November 21, 2024 07:57
@vcunat vcunat removed their request for review November 21, 2024 08:14
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Nov 21, 2024
@ofborg ofborg bot requested a review from lovek323 November 21, 2024 09:56
@getchoo getchoo added backport staging-24.11 Backport PR automatically and removed backport release-24.11 Backport PR automatically labels Nov 23, 2024
@niklaskorz
Copy link
Contributor

niklaskorz commented Dec 22, 2024

Curl has officially removed hyper support: https://daniel.haxx.se/blog/2024/12/21/dropping-hyper/

The hyper backend code has been removed in git as of December 21. There will be no traces left of it in the curl 8.12.0 release coming in February 2025.

@getchoo getchoo closed this Dec 22, 2024
@getchoo getchoo deleted the push-lqxkvwmnsxpz branch December 22, 2024 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

curl: move to hyper backend
5 participants