Skip to content

noice: init at 0.6 #27091

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

Merged
merged 1 commit into from
Jul 4, 2017
Merged

noice: init at 0.6 #27091

merged 1 commit into from
Jul 4, 2017

Conversation

jfrankenau
Copy link
Member

Motivation for this change

This is a small and fast TUI file browser.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

};

configFile = optionalString (conf!=null) (builtins.toFile "config.def.h" conf);
preBuild = optionalString (conf!=null) "cp ${configFile} config.def.h";
Copy link
Contributor

Choose a reason for hiding this comment

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

If you care to, I believe this could be simplified to echo ${conf} > config.h directly instead of creating an intermediate.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so too but somehow a lot of other packages are doing it the same way. Take abduco or st for example. Any good reason for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be that toFile handles large text chunks better. Other than that I fail to see the benefit of an intermediate here. Could be lack of imagination on my part, ofc :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I guess toFile is guaranteed to be "dumber" and so may be preferred.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've played around with echo but -- regardless of all the different kinds of quoting I tried -- somehow it didn't work out. Shouldn't the expression just be as follows?

preBuild = optionalString (conf!=null) "echo ${conf} > config.def.h";

conf itself was originally read by readFile in my overlays which should be a string. I guess there is a reason why toFile is preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like

optionalString (conf != null) ''echo "${conf}" > config.h''

I suppose a heredoc would also work, in cases where echo wouldn't. Anyway, if you prefer an intermediate, that's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried that and did some debugging. Looks like both echo ${conf} and echo "${conf}" are not correctly escaping the given config which contains characters like ">\. Only a heredoc solves this. Given these problems I'll keep it as it is.

@pSub pSub added the 8.has: package (new) This PR adds a new package label Jul 3, 2017
@joachifm joachifm merged commit 7e86d0e into NixOS:master Jul 4, 2017
@jfrankenau jfrankenau deleted the init-noice branch July 4, 2017 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants