treewide: consolidate logic in platform-specific commands #281
Conversation
|
I have neither standalone HM nor a Darwin configuration in my flake, so I can only hope those continue to work as intended. @khaneliman if you could see if this PR resolves the previous warning, that'd be neat. Feel free to also test other Darwin functionality. For now I'm going back to bed, this can probably be merged after proper testing and some rustdoc added to new functions (since they're a part of the util module.) |
|
mfw regressed again, god dammit |
NotAShelf
force-pushed
the
consolidate-logic
branch
from
61c4ef9 to
40025c3
Compare
NotAShelf
force-pushed
the
consolidate-logic
branch
from
40025c3 to
9bfb22f
Compare
We'll probably want to keep the code
|
This should be "stable" enough for testing now. As I haven't tested HM and Darwin functionality (knowing there are those who track the master branch) I am not merging this until I can get some test results. If only we had regression testing... |
|
I don't use a standalone home-manager, but I can test darwin and nixos. Seems to have resolved the warning about guessing hostname and building for a VM. But, it still has no diff output between generations. |
|
I've disabled diff in Darwin until I implement the new differ, though I might bring it back before this is merged. Does it not display diffs in current master? I'm off my computer and can't really look into the code rn. |
Nah, it doesn't on master either. I didn't realize it was a conscious choice, thought it was an accident. But, this does resolve the warning. |
|
It shouldn't be broken on master, at least I haven't noticed that it was. If you could bisect, I might be able to submit a patch later today or most likely tomorrow. Either way I'll bring it back with the new differ (#282) once I get to it. |
When I bisected it was on that PR I made the comment on, I think 477b751 ? . Right now I'm just pinning my flake input to the commit before it, merging the darwin root activation support. |
src/util/platform.rs
Outdated
| use crate::installable::Installable; | ||
|
|
||
| /// Resolves an Installable from an environment variable, or falls back to the provided one. | ||
| pub fn resolve_env_installable(var: &str, fallback: Installable) -> Installable { |
There was a problem hiding this comment.
Bikeshed: this should return an Option<Installable>, as the callsites favour doing platform::resolve_env_installable("NH_OS_FLAKE").unwrap_or(self.installable)
| /// - `config_name`: Optional configuration name (e.g. username@hostname) | ||
| /// - `push_drv`: Whether to push the drv path (platform-specific) | ||
| /// - `extra_args`: Extra args for nix eval (for config detection) | ||
| pub fn extend_installable_for_platform( |
There was a problem hiding this comment.
Maybe could have some unit tests.
There was a problem hiding this comment.
I'll probably rebase the tests PR on top of this one so that we can have unit tests, regression tests and property tests
| /// | ||
| /// The path to the built configuration, which can be used for activation | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub fn handle_rebuild_workflow( |
There was a problem hiding this comment.
I like doing a Builder for commands with many options, but this can go for now.
There was a problem hiding this comment.
What can go? The warning or the function?
|
Thank you for doing all of this 'manual labor', Raf. I agree something had to be done. One point that might cause an issue is that currently Then I'd could implement a fix for #299 on this branch. What do you think? Is it worth the extra complexity? |
Welcome back funny PR.
Something something consolidates duplicate platform logic something. Hopefully not a breaking change but I'm pushing this while incredibly sick and will go back to bed right after.