NixOS / nix

Nix, the purely functional package manager
https://nixos.org/
GNU Lesser General Public License v2.1
12.05k stars 1.47k forks source link

Antiquotes silently fail #1017

Open ttuegel opened 8 years ago

ttuegel commented 8 years ago

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 commented 8 years ago

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

edolstra commented 8 years ago

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

abbradar commented 8 years ago
 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 commented 8 years ago

:scream: :scream: :scream_cat:

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

abbradar commented 8 years ago

@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 commented 8 years ago

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

abbradar commented 8 years ago

First part done: https://github.com/NixOS/nixpkgs/commit/3fca2ce204909cd86fb4e713ca30d1c6a32b4d86 (at least those that I could catch with the regexp -- it's not ideal).

ttuegel commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

https://github.com/NixOS/nix/issues/836#issuecomment-194489542

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

copumpkin commented 8 years ago

Also note that @vcunat knew about this issue 😄 https://github.com/NixOS/nix/issues/836#issuecomment-194481343

abbradar commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

done

domenkozar commented 7 years ago

Another instance of this https://github.com/NixOS/nixpkgs/pull/19939

groxxda commented 7 years ago

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[bot] commented 3 years ago

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

stale[bot] commented 2 years ago

I closed this issue due to inactivity. → More info