resholve Add msmtp and msmtpq parsers

Here are some things that I’m not sure about:

I added more add_argument() calls than I needed to. I wanted to keep the code organized into the same categories that msmtp --help uses. I think that I could have combined a bunch of them like _tar() does. Would it be better if I did that? I was mainly trying to make sure that I didn’t accidentally skip over something.
I used nargs=1 for --passwordeval. msmtp’s manual makes it seem like I should have used nargs="?", but that doesn’t seem to work.

This breaks the parser, but I’m not sure why:

msmtp -O foo=bar

Log:

WARNING:__main__:CommandParser CommandParser(prog='msmtp (generic)', usage=None, description=None, version=None, formatter_class=<class 'argparse.HelpFormatter'>, conflict_handler='error', add_help=False) passing instead of 'argument -O/-o: expected 1 argument(s)'
  msmtp -O foo=bar
  ^~~~~
'/hdd/home/jayman/VC/Git/Partially mine/resholve/bugs/msmtp-test.sh':3: 'msmtp' executes its arguments in some circumstances, and none of my command-specific rules for figuring out if this specific invocation does or not succeeded. Look above for warning messages with more context on what went wrong.

Edit: This has been fixed.

Additionally, the msmtpq parser depends on abathur/binlore#12. That PR should be merged before this PR is merged.

Sep 06 '23 17:09 Jayman2000

I'm not certain on the error, but my first guess is that this has to do with the "varlike" (var=value) syntax in here.

resholve strips "varlikes" before trying to handle invocations for a few reasons, but the foremost is that shell/bash are so perverse that it's hard, from a very naive/static perspective, to know if these will be passed to the CLI or handled by the shell.

In addition to the normal shell behavior of strpping leading varlikes and using them to modify the environment of the invoked command, there's also a set flag that'll accept these everywhere:

If the -k option is set (see the set builtin command below), then all parameter assignments are placed in the environment for a command, not just those that precede the command name.

You can see this with something like:

$ ( set -k; echo KEY=WORD heh; )
heh

So, it's edge-case behavior, but if resholve leaves these, every single command parser could technically encounter them. Since I'm not aware of any that will contain commands themselves, I decided to see if we can live with just knocking these out.

The awk parser contains an example of how I've handled an arg that does indeed expect this form (and also an annoying corresponding short form):

https://github.com/abathur/resholve/blob/5e0956bd39a94159119647a13270841bb183f73e/resholve#L2009-L2018

Sep 08 '23 02:09 abathur

The awk parser contains an example of how I've handled an arg that does indeed expect this form (and also an annoying corresponding short form):

https://github.com/abathur/resholve/blob/5e0956bd39a94159119647a13270841bb183f73e/resholve#L2009-L2018

OK, I just pushed a new version of this PR that uses action="count".

Sep 11 '23 16:09 Jayman2000

Sorry for my inaction to turn into more busywork, but I wanted to see if you recall where this stands? It looks like I focused on the bigger issue and ignored the first two. Is the passwordeval/nargs bit still not working (and what did that mean, here)?

Grouping args isn't essential, especially since I'm not sure what the long-term best practice will be. I'd say: use whatever you think will be easiest to maintain. I can imagine making a case for at least a few ways of doing it, such as grouping similarly-handled arguments, ordering in man/--help/doc order, using whatever order the tool's own argument parsing code uses...

I'd also like to set up syntax parse tests for msmtp(q) like I did in the test branch I set up to work on improvements to unblock your flatpak PR. You don't have to add that here, but even if not I can still benefit from a little help staking out meaningful invocations to verify. (I was intending to just do these myself for both PRs, but I think working through the flatpak PR demonstrated that they're both good to have and that a quick skim of the docs won't necessarily be good enough for me to pick the right set to nail down.)

Mar 03 '24 22:03 abathur

Sorry for my inaction to turn into more busywork, but I wanted to see if you recall where this stands? It looks like I focused on the bigger issue and ignored the first two. Is the passwordeval/nargs bit still not working (and what did that mean, here)?

At the moment, I can’t get this PR to work. I test my PRs using ez-resholve.sh. Here’s what happens when I run that script with the version of resholve from this PR and the version of binlore from abathur/binlore#12:

$ export BINLORE=<path-to-binlore-repo> RESHOLVE=<path-to-resholve-repo>
$ ./ez-resholve.sh <script-that-uses-msmtp> msmtp hello
error:
       … while evaluating a branch condition

         at /nix/store/b8bwkgjwsrgjmfxpmcx84nkrjd37vw4d-nixos-23.11/nixos/lib/trivial.nix:386:27:

          385|   */
          386|   throwIfNot = cond: msg: if cond then x: x else throw msg;
             |                           ^
          387|

       … in the left operand of the OR (||) operator

         at /nix/store/b8bwkgjwsrgjmfxpmcx84nkrjd37vw4d-nixos-23.11/nixos/lib/trivial.nix:448:41:

          447|   */
          448|   isFunction = f: builtins.isFunction f ||
             |                                         ^
          449|     (f ? __functor && isFunction (f.__functor f));

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: value is a function while a set was expected
$

I tried rebasing this PR on resholve’s master branch and rebasing the other PR on binlore’s main branch, but that still didn’t make it work:

$ export BINLORE=<path-to-binlore-repo> RESHOLVE=<path-to-resholve-repo>
$ ./ez-resholve.sh <script-that-uses-msmtp> msmtp hello
/nix/store/if90y76azjvv5sh08kdryh77b6xqgfxh-more-binlore
/nix/store/408hlcs0avax5kfdbbxqlrammyryjpj8-resholve-0.10.2-924d60e
  example_email | msmtp -P --version -- --passwordeval=hello --passwordeval=hello --passwordeval=hello
                  ^~~~~
"/home/jayman/Documents/Home/VC/Git/Partially mine/resholve/msmtp-test.sh":20: I don't have any lore for '/nix/store/wak33dykfh5yb6l10jdh4lm193r3qchi-msmtp-1.8.22/bin/msmtp'
$

Now, resholve actually does have lore for msmtp, it just doesn’t have lore for the version of msmtp who’s hash is wak33dykfh5yb6l10jdh4lm193r3qchi. If I take a look at the lore that was generated:

$ cat  /nix/store/if90y76azjvv5sh08kdryh77b6xqgfxh-more-binlore/execers
can:/nix/store/i1svzazayprccdyk29pdfhxzlcv4ns6c-msmtp-1.8.22/bin/msmtp
can:/nix/store/i1svzazayprccdyk29pdfhxzlcv4ns6c-msmtp-1.8.22/bin/msmtpd
can:/nix/store/i1svzazayprccdyk29pdfhxzlcv4ns6c-msmtp-1.8.22/bin/msmtpq
can:/nix/store/i1svzazayprccdyk29pdfhxzlcv4ns6c-msmtp-1.8.22/bin/sendmail
cannot:/nix/store/v7f9znmg8yancpdhh4v82d6k8j9kqp0n-hello-2.12.1/bin/hello
$

Here’s what I think that the problem is. Resholve’s flake.lock file says

    "nixpkgs": {
      "locked": {
        "lastModified": 1713687659,
        "narHash": "sha256-Yd8KuOBpZ0Slau/NxFhMPJI0gBxeax0vq/FD0rqKwuQ=",
        "owner": "nixos",
        "repo": "nixpkgs",
        "rev": "f2d7a289c5a5ece8521dd082b81ac7e4a57c2c5c",
        "type": "github"
      },

Binlore’s flake.lock file says

    "nixpkgs": {
      "locked": {
        "lastModified": 1694948089,
        "narHash": "sha256-d2B282GmQ9o8klc22/Rbbbj6r99EnELQpOQjWMyv0rU=",
        "owner": "nixos",
        "repo": "nixpkgs",
        "rev": "5148520bfab61f99fd25fb9ff7bfbb50dad3c9db",
        "type": "github"
      },

I think that resholve is looking at /nix/store/wak33dykfh5yb6l10jdh4lm193r3qchi-msmtp-1.8.22/bin/msmtp and binlore is looking at /nix/store/i1svzazayprccdyk29pdfhxzlcv4ns6c-msmtp-1.8.22/bin/msmtp because they’re using different verisons of Nixpkgs.

Grouping args isn't essential, especially since I'm not sure what the long-term best practice will be. I'd say: use whatever you think will be easiest to maintain. I can imagine making a case for at least a few ways of doing it, such as grouping similarly-handled arguments, ordering in man/--help/doc order, using whatever order the tool's own argument parsing code uses...

I ordered them the way msmtp --help does. I’ll leave it that way because I think that it makes the code easier to follow.

I'd also like to set up syntax parse tests for msmtp(q) like I did in the test branch I set up to work on improvements to unblock your flatpak PR. You don't have to add that here, but even if not I can still benefit from a little help staking out meaningful invocations to verify. (I was intending to just do these myself for both PRs, but I think working through the flatpak PR demonstrated that they're both good to have and that a quick skim of the docs won't necessarily be good enough for me to pick the right set to nail down.)

How do I create tests?

May 03 '24 13:05 Jayman2000

At the moment, I can’t get this PR to work. I test my PRs using ez-resholve.sh. Here’s what happens when I run that script with the version of resholve from this PR and the version of binlore from abathur/binlore#12:

I see what's going on here. Short-ish answer is that they aren't necessarily updated in lock step; resholve's flake.nix controls for this by setting binlore.inputs.nixpkgs.follows = "nixpkgs"; (i.e., it's overriding binlore's nixpkgs to use resholve's). I agree that there should be an easy (or at least easier) way to do what you're trying to do, but there's a bit of a blindspot in flakes around a project's ability to define a shell environment that ~users can readily pass arbitrary packages into.

I'm not sure if there's a low-pain fix off the top of my head, but I'll start looking at how to support the use-case as soon as I post this reply.

(Shooting from the hip, I imagine the simplest way to make this work now is probably to make a small flake that consumes resholve and override both resholve and resholve.binlore to the versions you want to test, and then just use one of resholve's normal Nix functions to handle your test script. That said, that'll probably only unblock you if you're already familiar with flakes...)

I ordered them the way msmtp --help does. I’ll leave it that way because I think that it makes the code easier to follow.

👍

How do I create tests?

IIRC that msmtp(q) are generally available on unix, it'll mostly mean:

adding a tests/parse_msmtp.sh file. I'm not sure if it makes sense to add both of these in one file or to separate them.

Here's an example of what these look like: https://github.com/abathur/resholve/blob/master/tests/parse_sed.sh. It's still pretty ad-hoc, so I haven't figured out how to deal with negative syntax cases (those we actually expect to break on) yet, but it's still good to leave some of those commented out if you know what they are. The goal is to sanity-check the parser on known-good invocations, so at least for now it'll actually invoke these.

Note: I caveated "generally available on unix" earlier because the master branch doesn't currently have support for running these conditionally based on platform. There's a rough implementation of that in the branch I was using to play around with the flatpak PR, so I can extract and land it if needed.
add the package(s) to the list of parsed_packages in nixpkgs/test.nix: https://github.com/abathur/resholve/blob/4715114c59c4484e8a832ee973753a237010f004/nixpkgs/test.nix#L65

May 03 '24 19:05 abathur

At the moment, I can’t get this PR to work. I test my PRs using ez-resholve.sh. Here’s what happens when I run that script with the version of resholve from this PR and the version of binlore from abathur/binlore#12:

I see what's going on here. Short-ish answer is that they aren't necessarily updated in lock step; resholve's flake.nix controls for this by setting binlore.inputs.nixpkgs.follows = "nixpkgs"; (i.e., it's overriding binlore's nixpkgs to use resholve's). I agree that there should be an easy (or at least easier) way to do what you're trying to do, but there's a bit of a blindspot in flakes around a project's ability to define a shell environment that ~users can readily pass arbitrary packages into.

I'm not sure if there's a low-pain fix off the top of my head, but I'll start looking at how to support the use-case as soon as I post this reply.

Ok! It's ugly, but I pulled something together in https://github.com/abathur/resholve/commit/dc88cc92cf79a141c81f2a6df003025dcb27c0c8 that should be workable as long as using the new CLI/flakes isn't a dealbreaker for you.

Basically, I've added a ~runnable package to the flake that is itself a shell script that'll build some Nix expressions to let us dynamically pass in a list of packages to use here.

It looks like this:

$ nix run .#resholve-with-packages -- tests/parse_sed.sh gnused
warning: Nix search path entry '/nix/var/nix/profiles/per-user/root/channels' does not exist, ignoring
warning: Nix search path entry '/nix/var/nix/profiles/per-user/root/channels' does not exist, ignoring
  sed -E -e "s,.*(query\\[A|DHCP).*,${COL_NC}&${COL_NC},"
            ^
[ stdin ]:13: CAUTION: My sed parser couldn't handle this expression. 

Next steps:
- Manually check it for exec (e commands) and patch any you find.
- Report your case in the issue to help us understand these cases

https://github.com/abathur/resholve/issues/28
#!/nix/store/386xmpmnix11qhnn2l0f7lvv0dc70w8h-bash-5.2p26/bin/bash
echo heh > infile
/nix/store/1qwza8ysi153w6v88sfnn6f4y4j8zcag-gnused-4.9/bin/sed -e 's/e/a/' infile
# TODO: when you've got test scaffold support for error cases, add true e commands
# echo heh | sed -e 'e echo hah'
echo bleh | /nix/store/1qwza8ysi153w6v88sfnn6f4y4j8zcag-gnused-4.9/bin/sed 's/e/a/g'
echo blah | /nix/store/1qwza8ysi153w6v88sfnn6f4y4j8zcag-gnused-4.9/bin/sed 's/.*:/,/;s/$/,/'
# throw away status because /mnt/etc
# won't be available for the test
/nix/store/1qwza8ysi153w6v88sfnn6f4y4j8zcag-gnused-4.9/bin/sed -i "1p" /mnt/etc || true

# extracted/simplified from pihole
/nix/store/1qwza8ysi153w6v88sfnn6f4y4j8zcag-gnused-4.9/bin/sed -E -e "s,.*(query\\[A|DHCP).*,${COL_NC}&${COL_NC},"
# patched version of above that should actually parse
/nix/store/1qwza8ysi153w6v88sfnn6f4y4j8zcag-gnused-4.9/bin/sed -E -e 's,.*(query\[A|DHCP).*,'"${COL_NC}&${COL_NC},"

### resholve directives (auto-generated) ## format_version: 3
# resholve: keep /nix/store/1qwza8ysi153w6v88sfnn6f4y4j8zcag-gnused-4.9/bin/sed

Not married to the current name (it was a hasty pre-commit choice...) and open to haggling over the right argument syntax.

For the first draft I just passed the script in on stdin so that it won't be writing files by default.

Now that I've pushed it, I guess we might be able to just handle the package args and suggest passing the script on stdin from outside of the invocation? Thoughts?

I don't need to cut a release to push this out on master--I can do so as soon as we're settled here.

May 04 '24 01:05 abathur

Ok! It's ugly, but I pulled something together in dc88cc92cf79a141c81f2a6df003025dcb27c0c8 that should be workable as long as using the new CLI/flakes isn't a dealbreaker for you.

I tried rebasing this PR on that commit, and it worked!

Now that I've pushed it, I guess we might be able to just handle the package args and suggest passing the script on stdin from outside of the invocation? Thoughts?

I personally prefer having a script argument. If there wasn’t a script argument, then it would turn my current workflow:

nix run .#resholve-with-packages -- ../script.sh hello

into this:

cat ../script.sh | nix run .#resholve-with-packages -- hello

Keeping it as an argument is just slightly less for me to type.

I don't need to cut a release to push this out on master--I can do so as soon as we're settled here.

OK. Once that script gets pushed to master, I’ll rebase this PR on master and then write tests.

May 08 '24 21:05 Jayman2000

Pushed it a moment ago.

May 09 '24 14:05 abathur

OK. I just pushed a new rebased version of this PR that adds tests.

May 09 '24 17:05 Jayman2000

Meant to get to this on Friday but we've been displaced and without power since Thursday night and it's left me a bit scattered :)

Not sure when I'll cut a release including this.

May 22 '24 12:05 abathur