This is because a dynamic_cast<nix::RootArgs *> of a (n-e-j) MyArgs
returns nullptr even though MyArgs has virtual nix::RootArgs as a
parent.
class MyArgs : virtual public nix::MixEvalArgs,
virtual public nix::MixCommonArgs,
virtual nix::RootArgs { ... };
So this should work right?? But it does not. We found out that it's
caused by -fvisibility=hidden in n-e-j, but honestly this code was bad
anyway.
The trivial solution is to simply stop relying on RTTI working properly
here, which is probably better OO architecture anyway. However, I am not
100% confident *this* is sound, since we have this horrible hierarchy:
Args (defines getRoot)
/ | \
RootArgs MixCommonArgs MixEvalArgs
(overrides)
I am not confident that this is guaranteed to resolve from Args always
in the case of this override.
Assertion failed: (res), function getRoot, file src/libutil/args.cc, line 67.
6MyArgsProcess 60503 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
frame #4: 0x0000000100b1a41c liblixutil.dylib`nix::Args::processArgs(std::__1::list<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>> const&, bool) [inlined] nix::Args::getRoot(this=0x00000001000d0688) at args.cc:67:5 [opt]
64 std::cout << typeid(*p).name();
65
66 auto * res = dynamic_cast<RootArgs *>(p);
-> 67 assert(res);
68 return *res;
69 }
70
Target 0: (nix-eval-jobs) stopped.
(lldb) p this
(MyArgs *) 0x00000001000d0688
(lldb) p *this
(nix::Args) {
longFlags = size=180 { ... }
shortFlags = size=4 { ... }
expectedArgs = size=1 { ... }
processedArgs = size=0 {}
hiddenCategories = size=1 {
[0] = "Options to override configuration settings"
}
parent = nullptr
}
We also found that if we did this:
class [[gnu::visibility("default")]] RootArgs : virtual public Args
it would work properly (???!). This is of course, very strange, because
objdump -Ct output on liblixexpr.dylib is identical both with and
without it.
Possibly related: https://www.qt.io/blog/quality-assurance/one-way-dynamic_cast-across-library-boundaries-can-fail-and-how-to-fix-it
Fixes: lix-project/nix-eval-jobs#2
Change-Id: I6b9ed968ed56420a9c4d2dffd18999d78c2761bd
As I complained in
https://github.com/NixOS/nix/pull/6784#issuecomment-1421777030 (a
comment on the wrong PR, sorry again!), #6693 introduced a second
completions mechanism to fix a bug. Having two completion mechanisms
isn't so nice.
As @thufschmitt also pointed out, it was a bummer to go from `FlakeRef`
to `std::string` when collecting flake refs. Now it is `FlakeRefs`
again.
The underlying issue that sought to work around was that completion of
arguments not at the end can still benefit from the information from
latter arguments.
To fix this better, we rip out that change and simply defer all
completion processing until after all the (regular, already-complete)
arguments have been passed.
In addition, I noticed the original completion logic used some global
variables. I do not like global variables, because even if they save
lines of code, they also obfuscate the architecture of the code.
I got rid of them moved them to a new `RootArgs` class, which now has
`parseCmdline` instead of `Args`. The idea is that we have many argument
parsers from subcommands and what-not, but only one root args that owns
the other per actual parsing invocation. The state that was global is
now part of the root args instead.
This did, admittedly, add a bunch of new code. And I do feel bad about
that. So I went and added a lot of API docs to try to at least make the
current state of things clear to the next person.
--
This is needed for RFC 134 (tracking issue #7868). It was very hard to
modularize `Installable` parsing when there were two completion
arguments. I wouldn't go as far as to say it is *easy* now, but at least
it is less hard (and the completions test finally passed).
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Change-Id: If18cd5be78da4a70635e3fdcac6326dbfeea71a5
(cherry picked from commit 67eb37c1d0de28160cd25376e51d1ec1b1c8305b)
Improve tests and docs prior to refactoring completions
(cherry picked from commit 5442d9b47298389918d1f38d20f768a80ffc2369)
Change-Id: Ief99ac2cd9c92981a9a522d15b9c3daf99182c9d
Prior to this, there was an ad-hoc whitelist in `main.cc`. Now, every
command states its stability.
In a future PR, we will adjust the manual to take advantage of this new
information in the JSON.
(It will be easier to do that once we have some experimental feature
docs to link too; see #5930 and #7798.)
If we conditionally "declare" the argument, as we did before, based upon
weather the feature is enabled, commands like
nix --experimental-features=foo ... --thing-gated-on-foo
won't work, because the experimental feature isn't enabled until *after*
we start parsing.
Instead, allow arguments to also be associated with experimental
features (just as we did for builtins and settings), and then the
command line parser will filter out the experimental ones.
Since the effects of arguments (handler functions) are performed right
away, we get the required behavior: earlier arguments can enable later
arguments enabled!
There is just one catch: we want to keep non-positional
flags...non-positional. So if
nix --experimental-features=foo ... --thing-gated-on-foo
works, then
nix --thing-gated-on-foo --experimental-features=foo ...
should also work.
This is not my favorite long-term solution, but for now this is
implemented by delaying the requirement of needed experimental features
until *after* all the arguments have been parsed.
Already, we had classes like `BuiltPathsCommand` and `StorePathsCommand`
which provided alternative `run` virtual functions providing the
implementation with more arguments. This was a very nice and easy way to
make writing command; just fill in the virtual functions and it is
fairly clear what to do.
However, exception to this pattern were `Installable{,s}Command`. These
two classes instead just had a field where the installables would be
stored, and various side-effecting `prepare` and `load` machinery too
fill them in. Command would wish out those fields.
This isn't so clear to use.
What this commit does is make those command classes like the others,
with richer `run` functions.
Not only does this restore the pattern making commands easier to write,
it has a number of other benefits:
- `prepare` and `load` are gone entirely! One command just hands just
hands off to the next.
- `useDefaultInstallables` because `defaultInstallables`. This takes
over `prepare` for the one case that needs it, and provides enough
flexiblity to handle `nix repl`'s idiosyncratic migration.
- We can use `ref` instead of `std::shared_ptr`. The former must be
initialized (so it is like Rust's `Box` rather than `Option<Box>`,
This expresses the invariant that the installable are in fact
initialized much better.
This is possible because since we just have local variables not
fields, we can stop worrying about the not-yet-initialized case.
- Fewer lines of code! (Finally I have a large refactor that makes the
number go down not up...)
- `nix repl` is now implemented in a clearer way.
The last item deserves further mention. `nix repl` is not like the other
installable commands because instead working from once-loaded
installables, it needs to be able to load them again and again.
To properly support this, we make a new superclass
`RawInstallablesCommand`. This class has the argument parsing and
completion logic, but does *not* hand off parsed installables but
instead just the raw string arguments.
This is exactly what `nix repl` needs, and allows us to instead of
having the logic awkwardly split between `prepare`,
`useDefaultInstallables,` and `load`, have everything right next to each
other. I think this will enable future simplifications of that argument
defaulting logic, but I am saving those for a future PR --- best to keep
code motion and more complicated boolean expression rewriting separate
steps.
The "diagnostic ignored `-Woverloaded-virtual`" pragma helps because C++
doesn't like our many `run` methods. In our case, we don't mind the
shadowing it all --- it is *intentional* that the derived class only
provides a `run` method, and doesn't call any of the overridden `run`
methods.
Helps with https://github.com/NixOS/rfcs/pull/134
Defers completion of flake inputs until the whole command line is parsed
so that we know what flakes we need to complete the inputs of.
Previously, `nix build flake --update-input <Tab>` always behaved like
`nix build . --update-input <Tab>`.
To quote Eelco in #5867:
> Unfortunately we can't do
>
> evalSettings.pureEval.setDefault(false);
>
> because then we have to do the same in main.cc (where
> pureEval is set to true), and that would allow pure-eval
> to be disabled globally from nix.conf.
Instead, a command should specify that it should be impure by
default. Then, `evalSettings.pureEval` will be set to `false;` unless
it's overridden by e.g. a CLI flag.
In that case it's IMHO OK to be (theoretically) able to override
`pure-eval` via `nix.conf` because it doesn't have an effect on commands
where `forceImpureByDefault` returns `false` (i.e. everything where pure
eval actually matters).
Closes#5867
This is probably what most people expect it to do. Fixes#3781.
There is a new command 'nix flake lock' that has the old behaviour of
'nix flake update', i.e. it just adds missing lock file entries unless
overriden using --update-input.
This is technically a breaking change, since attempting to set plugin
files after the first non-flag argument will now throw an error. This
is acceptable given the relative lack of stability in a plugin
interface and the need to tie the knot somewhere once plugins can
actually define new subcommands.
Make nix output completions in the form `completion\tdescription`.
This can't be used by bash (afaik), but other shells like zsh or fish
can display it along the completion choices
jade
Tom Hubrecht
eldritch horrors
Maximilian Bosch