Lorri upgrade refactor and branch subcommand #168
Conversation
Profpatsch
force-pushed
the
lorri-upgrade-refactor-and-branch-subcommand
branch
from
bbcc370 to
ca009d6
Compare
Profpatsch
force-pushed
the
lorri-upgrade-refactor-and-branch-subcommand
branch
from
ca009d6 to
3f2fe44
Compare
There was a problem hiding this comment.
The use case for self-upgrade is not clear to me.
- If I'm just a normal user of
lorri, I'll want to install it from a package repository. - If I'm a sophisticated user living on the bleeding edge, I'll figure out that I need to
git clone ; cargo buildeasily enough.
| fetchedSource = | ||
| if type == "branch" then fetchBranch branch | ||
| else if type == "local" then path | ||
| else abort "impossibru"; |
There was a problem hiding this comment.
Cute, but not very helpful!
There was a problem hiding this comment.
easy to grep for :)
We have users who would like to use lorri without going through the indirection of nixpkgs (and who don’t want to fetch a whole nixpkgs just to update lorri). Having lorri in nixpkgs is a nice addition, but it’s way easier for us to tell people “run |
Profpatsch
force-pushed
the
lorri-upgrade-refactor-and-branch-subcommand
branch
from
3f2fe44 to
55f5259
Compare
Profpatsch
force-pushed
the
lorri-upgrade-refactor-and-branch-subcommand
branch
from
55f5259 to
bb0af83
Compare
There was a problem hiding this comment.
I still believe it is overkill to build a sort of build system for lorri into lorri when it's not hard to do the same just by invoking
nix-env -if https://github.com/target/lorri/archive/(rolling-release|commit hash|...).tar.gzBut if the rest of the team believes this is valuable, I won't oppose it.
My remaining questions are:
- What's the use case for the
localbuild? In that case the user has already cloned the repo, possibly made some local changes, and probably built and tested their changes. At this point, what is the benefit of invoking alorricommand to install this local version of lorri over a Nix command to do the same? - Where will this build to? Will it override anything?
I believe that an integration test would be very useful to have to make sure this reasonably complex combination of Nix and Rust works and that subtleties like "releaseNix may or may not take a src attribute" are handled correctly.
|
This hasn't been updated in a very long time. Personally i'm a bit in doubt about this and if this should be included in lorri. The PR description doesn't add any motivation unfortunately but I think Beyond that it's open source and people can build whatever branch or revision they want to. |
|
The motivation here is to make it easy for users to try out a branch that devs push, so that they can verify whether it fixes their problems. Instead of “check out the repo and run these nix commands” we just tell them “run |
|
Since it’s a rather simple incremental change over our current self-upgrade command with a clear improvement for user interaction, I expected it to be fairly uncontroversial back when I submitted it. But maybe I should have given a better motivation indeed. |
|
fixes #446 |
|
I can say that I have a lot of bugs to respond to being a user of emacs, nix, direnv, ghcide, etc. I also have a high tolerance for thinking with broken things or getting stuck am hour debugging a few simple commands that "should have worked". At the time I ran the lorri upgrade command to test because it was so easy to do. I think it makes it a lot more likely for other users to participate in testing that don't have as high of a tolerance for tinkering. |
Removes one instance of old `mod.rs` scheme.
Profpatsch
force-pushed
the
lorri-upgrade-refactor-and-branch-subcommand
branch
from
bb0af83 to
f624615
Compare
This rewrites the upgrade.nix code to accept arbitrary branch names (instead of just `master` and `rolling-release`). It introduces a local `UpgradeSource` struct in `upgrade/mod.rs`, which contains only the interesting information we need for calling `upgrade.nix`, that is whether we want a local checkout or a branch. The given command line arguments get parsed correctly instead of being passed straight to `./upgrade.nix`, with better error messages.
This means we can easily tell users to try out various branches with fixes for their problems, for easier debugging.
We shouldn’t pass src explicitely (because it can lead to a mismatch between `default.nix` and source code), and indeed we never need to, not even for `release.nix` and `lorri self-upgrade`. We have to add an `isFunction` check, otherwise `self-upgrade` fails for branches where the `src` attribute is still passed manually to `release.nix`.
We only want to read the `release.nix` file from the fetched source repository, not import it into the store recursively; thus, `toString`. `release.nix` will still copy the source to the store, but it filters src correctly, so e.g. the `.git` directory is not copied to the store.
nix is content with arbitrary bytestrings, which is nice if you have e.g. a path that contains non-UTF-8 characters. So we don’t require a rust `str`, rather an OsStr(ing). It can still be used with normal strings, because of the `AsRef` instances.
Profpatsch
force-pushed
the
lorri-upgrade-refactor-and-branch-subcommand
branch
from
f624615 to
e068b0b
Compare
|
I rebased and addressed the remaining comments. I’m gonna merge it, whether or what part of the module we want to remove is for a different PR. |
Adds
lorri self-upgrade branchand makes the parsing more solid.