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.

Sign up for GitHub

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

Jump to bottom

fetchurl: strip query from filename #107515

Closed
wants to merge 2 commits into from
Closed

fetchurl: strip query from filename #107515

wants to merge 2 commits into from

Conversation

holymonson
Copy link
Contributor

@holymonson holymonson commented Dec 24, 2020 •
edited

Motivation for this change

When fetching url like https://bug-220081-attachments.webkit.org/attachment.cgi?id=416707&action=diff&format=raw , the filename contains query separator & which is invalid as path name in Nix. In fact, since it's not part of path according to URI syntax, we can simply strip the whole query component.

Things done

Manually fetchpatch the patch mention above and it works.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@holymonson holymonson marked this pull request as draft December 24, 2020 02:52
infinisil
infinisil requested changes Dec 24, 2020
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Nice!

pkgs/build-support/fetchurl/default.nix Outdated Show resolved Hide resolved
@holymonson holymonson marked this pull request as ready for review December 24, 2020 07:05
@holymonson holymonson force-pushed the fetchurl branch 2 times, most recently from 4464ebb to 1410bc0 Compare December 24, 2020 09:57
@SuperSandro2000
Copy link
Member

Please target staging.

infinisil
infinisil reviewed Dec 24, 2020
View reviewed changes
lib/strings.nix Outdated Show resolved Hide resolved
@holymonson holymonson force-pushed the fetchurl branch 2 times, most recently from 5f6ced2 to a567b92 Compare December 25, 2020 05:13
@holymonson holymonson changed the base branch from master to staging December 25, 2020 06:51
@infinisil infinisil mentioned this pull request Dec 30, 2020
Closed
@infinisil
Copy link
Member

I just wrote the related NixOS/rfcs#84, where I intentionally pointed out that all fetchers should use the same name, so that the store path can be reused between them. In this case, we also have nix-prefetch-url which currently fails with the same error as pkgs.fetchurl, builtins.fetchurl or <nix/fetchurl.nix>. This PR would make pkgs.fetchurl not fail where nix-prefetch-url, builtins.fetchurl and <nix/fetchurl.nix> still would.

I think let's first see where the discussion in NixOS/rfcs#84 goes, because depending on that, this PR might not be necessary, or this PR's behavior could also be applied to all other URL fetchers.

@stale
Copy link

stale bot commented Jul 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@drupol
Copy link
Contributor

drupol commented Jun 6, 2023

@holymonson Do you think we can close this PR?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 6, 2023
@holymonson holymonson closed this Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

@infinisil infinisil Awaiting requested review from infinisil infinisil is a code owner

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: fetch 10.rebuild-darwin: 501+ 10.rebuild-darwin: 1001-2500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@holymonson @SuperSandro2000 @infinisil @drupol @r-ryantm