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

Antiquotes silently fail #1017

Open
ttuegel opened this issue Aug 8, 2016 · 21 comments
Open

Antiquotes silently fail #1017

ttuegel opened this issue Aug 8, 2016 · 21 comments
Assignees
Labels

Comments

@ttuegel
Copy link
Member

ttuegel commented Aug 8, 2016

See NixOS/nixpkgs#17595. An antiquote of the form

'' ${LINK:0:1} ''

substitutes the string LINK:0:1 into the builder, instead of failing because this is not valid Nix syntax.

@abbradar
Copy link
Member

abbradar commented Aug 8, 2016

I wonder just how many evaluation errors we'd catch when this is fixed...

@edolstra
Copy link
Member

edolstra commented Aug 8, 2016

It is in fact valid Nix syntax, since link:0:1 is parsed as a URI.

@abbradar
Copy link
Member

abbradar commented Aug 8, 2016

 nixpkgs git:(master) ✗ egrep --include=\*.nix -r "[^'\\]\\\${[a-zA-Z0-9]+:"
nixos/modules/system/boot/stage-1.nix:            if [ "${LINK:0:1}" != "/" ]; then
pkgs/development/tools/misc/hydra/default.nix:    export LOGNAME=${LOGNAME:-foo}
pkgs/servers/amqp/rabbitmq-server/default.nix:      echo 'PATH=${erlang}/bin:${PATH:+:}$PATH' >> $out/sbin/rabbitmq-env

@ttuegel
Copy link
Member Author

ttuegel commented Aug 8, 2016

😱 😱 🙀

@abbradar Wow, your regex-fu... Much respect.

@abbradar
Copy link
Member

abbradar commented Aug 8, 2016

@ttuegel I won't be able to parse it myself in I think 15 minutes ~_^.

Seems an ugly situation without an obvious solution -- enough to tempt me to add some scream smileys to the row. Any ideas?

@ttuegel
Copy link
Member Author

ttuegel commented Aug 8, 2016

I would say, fix all the occurrences in Nixpkgs and add a linting pass to Travis CI.

abbradar added a commit to NixOS/nixpkgs that referenced this issue Aug 8, 2016
@abbradar
Copy link
Member

abbradar commented Aug 8, 2016

First part done: NixOS/nixpkgs@3fca2ce (at least those that I could catch with the regexp -- it's not ideal).

@ttuegel
Copy link
Member Author

ttuegel commented Aug 8, 2016

The only other thing I can think of is disallowing literals from antiquotes. The only valid reason to use a literal that I can think of is to copy an in-tree path to the store. That would definitely be the "scorched earth" approach, though.

@abbradar
Copy link
Member

abbradar commented Aug 8, 2016

We can just disallow URLs in them -- but I'm not sure it could be done easily in Nix (I imagine they are converted to just strings in the parser). If paths are parsed differently however we can disallow strings in antiquotes entirely (I don't know a good usecase for ${"blah"}).

EDIT: to clarify, I propose to have this test before any reduction, so ${makeLibraryPath [foo]} should be fine.

@copumpkin
Copy link
Member

#836 (comment)

I'm still not sure I buy the value of having special complex URI syntax over writing two extra quote characters for URLs.

@copumpkin
Copy link
Member

Also note that @vcunat knew about this issue 😄 #836 (comment)

@abbradar
Copy link
Member

abbradar commented Aug 8, 2016

IIUC @vcunat was not talking about this issue, but about absence of antiquotes in URLs (http://foo/${bar}).

I happen to agree that URI syntax is too much pain with parsing, tooling and issues like this for too little gain.

Nikolay.

@vcunat
Copy link
Member

vcunat commented Aug 9, 2016

Abbradar is correct in what I meant.

The non-URL URI literals really do seem to be confusing to most people and I don't know of any use case for them.

@fkz
Copy link
Contributor

fkz commented Aug 9, 2016

It would be pretty easy to disallow uri syntax inside ${ ... } blocks. I don't see much value in them there.
On the other hand adding special cases doesn't make learning nix easier

@fkz
Copy link
Contributor

fkz commented Aug 9, 2016

just implemented this and found an other bug this way: https://github.com/NixOS/nixpkgs/blob/7475728593a49f09c4b7b959b15513aee38ab4b4/pkgs/games/vessel/default.nix#L18 that your regex foo didn't seem to be catching

@abbradar
Copy link
Member

abbradar commented Aug 9, 2016

Awesome! Instead of improving the abomination I suppose we'd forbid URIs inside antiquotes and be done with it. Please, open a PR.

EDIT: by abomination I meant my regex ~_^

fkz added a commit to fkz/nix that referenced this issue Aug 9, 2016
this fixes NixOS#1017
evaluating `nix-env -qa` and (modulo some typo problem) this doesn't seem to be used inside nixpkgs.
this is a non-backward-compatible change though, 3rd party code could break because of this.
@fkz
Copy link
Contributor

fkz commented Aug 9, 2016

done

@domenkozar
Copy link
Member

Another instance of this NixOS/nixpkgs#19939

@groxxda
Copy link
Contributor

groxxda commented Oct 28, 2016

I vote for dropping native uri parsing since it does not support the full rfc anyway: Two valid uris that are not recognized: http://[::1] and http://localhost/name;with;semicolons

@stale
Copy link

stale bot commented Feb 15, 2021

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

@stale stale bot added the stale label Feb 15, 2021
@stale
Copy link

stale bot commented May 2, 2022

I closed this issue due to inactivity. → More info

@stale stale bot closed this as completed May 2, 2022
@thufschmitt thufschmitt reopened this Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants