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

Fix the parsing of the query of path-like flakerefs #10125

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thufschmitt
Copy link
Member

Motivation

On current master, a flake-like pathref with a query but no fragment will quietly drop the query.
For instance, nix build .?rev=c43c75ea7983f84270cfdcca18169c03987287a4 will quietly ignore the rev parameter and behave like nix build ..

Fix that, and add some tests.

Context

Spotted as part of #6633.
Probably introduced by #6614.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@thufschmitt thufschmitt requested a review from edolstra as a code owner March 1, 2024 16:41
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Mar 1, 2024
Théophane Hufschmitt added 3 commits March 2, 2024 10:11
Make sure that various combinations of query strings, fragments, relative
and absolute paths behave properly.
The query parameters for path flakerefs (`/foo/bar?query=blah` or
`.?foo=bar`) was ignored when the path wasn't a git repo.
Fix that
`/path?foo=bar` was parsed as `/path`, omitting the query altogether
(`/path?foo=bar#` was parsed correctly though).

Rewrite the path flake ref parser to fix this
@thufschmitt thufschmitt force-pushed the fix-path-flakeref-query-parsing branch from 882e139 to 3740cdb Compare March 2, 2024 09:12
@thufschmitt
Copy link
Member Author

Rewrote the test to directly test the flakeRef C++ function rather than the primop because it's much easier to introspect the C++ values than the Nix-language ones (and that avoids having to fiddle with the xp feature settings)

@kubukoz kubukoz mentioned this pull request Mar 2, 2024
@thufschmitt
Copy link
Member Author

@Ericson2314 I'm a bit stuck on the tests here because they require Xp::Flakes. Is there any way to work around it? (I know some code explicitly passes the experimental features set for testing purposes, but fetchers don't, and I don't want to rewrite it just for that simple test).

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-55/40996/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants