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

[WIP] +RFC fixing arguments parsing and documentation for nix-build and nix-shell #2346

Closed
wants to merge 4 commits into from

Conversation

samueldr
Copy link
Member

@samueldr samueldr commented Aug 12, 2018

nix-build and nix-shell are both implemented in the same multi-call binary.

The current implementation for arguments parsing makes them share arguments. This can lead to surprising behaviour (or lack thereof) when accidentally using a non-shared parameter.

Abridged list of nonsensical invocations:

  • nix-build --command "foo" ./release.nix
  • nix-build --impure ./release.nix -A build.x86_64-linux
  • nix-shell --out-link "foo" ./shell.nix

There are a couple more that could be added.

This PR intends to fix the behaviour by segregating the arguments parsing using an if conditional. That is, unless a better method is suggested.

Additionally, this PR adds the missing documentation for a couple of options, since while checking for the "owner" of the parameters, I realized that a couple options weren't documented.

Expectations

I am expecting a bit of push-back from "simply removing" the options. That is, it technically breaks backwards-compatibility with (wrong) usage of the 2.x series for nix-build and nix-shell. This has to be kept in mind.

If it is judged better to keep the non-sensical options, it may be preferred to keep them segregated in their respective binaries, turned into no-ops in the other and documented as such.

RFC stdin

I find the - (stdin) behaviour surprising with regards to nix-shell.

  • nix-shell ./shell.nix != echo 'import ./shell.nix' | nix-shell -

As for nix-build it seems the behaviour is fine.

I am probably overlooking something with the semantics of it. Though, the way it's implemented it may be better to drop this flag in nix-shell? The only documentation for - as standard input is for nix-instantiate, and there are no mention of it in neither the nix-build nor the nix-shell manuals. (They mention nix-instantiate. only for --arg | --attr | -A.


I may split this PR into a documentation-only one that can be fast-tracked. I may also split this PR furthermore while it's WIP.


Furthermore, this will (still a WIP) try to address #1982 and #2208 by actually implementing the flag.

@dtzWill
Copy link
Member

dtzWill commented Sep 23, 2018

❤️

@lheckemann
Copy link
Member

Any reason to attempt to address #2208 in this PR rather than only separating the options and implementing the shell-gc-rooting stuff in another one? With that considered, is this still WIP?

@samueldr
Copy link
Member Author

samueldr commented Nov 8, 2018

The only reason I thought about #2208 was that it was possible to do it while passing there. It wasn't the initial goal of the PR, and yeah, probably should be handled in another PR.

The actual PR, "fixing arguments parsing and documentation" isn't much WIP, but more "tell me what I'm doing wrong". See the "Expectations" and "RFC stdin" sections for other parts where I'm not entirely confident.

@asymmetric
Copy link
Contributor

asymmetric commented Jun 7, 2019

I think it would make sense to split out the documentation part from the PR and fast-track it.

Additionally, I think it would be better to handle #2208 in a separate PR too.

This has been sitting around for a while already and the option housekeeping stuff is pretty useful!

@domenkozar
Copy link
Member

domenkozar commented Apr 19, 2020

I think such PRs would be eaiser to merge if they come with a bunch of tests :)

I agree with @asymmetric on the docs.

@stale
Copy link

stale bot commented Feb 13, 2021

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

@stale stale bot added the stale label Feb 13, 2021
@samueldr samueldr closed this Feb 13, 2021
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 this pull request may close these issues.

5 participants