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

notes from nixpkgs#173885 #80

Open
abathur opened this issue May 26, 2022 · 5 comments
Open

notes from nixpkgs#173885 #80

abathur opened this issue May 26, 2022 · 5 comments

Comments

@abathur
Copy link
Owner

abathur commented May 26, 2022

I'll give you a step-for-step introduction of what I did and what I found, maybe this can help you to help me:

I first took a look at Kokadas' derivation and saw that it was primarily rounded on that "solutions" entry after adding the "mkDerivation". So I searched how to write one of those, which led me to this README that I've used as the source of truth.

My first doubt was, "Should I use the paths relative to input or output?" and well, that was written there, "$out-relative paths".

The second was an observation that I've seen in Kokadas' code. He was using "sh" instead of "bash" as interpreter, which led me to ask him and then discovering bash includes a posix-mode.

Then a small try after just adding the "scripts", "interpreter", and the "inputs" I've already known lead me to that regex parsing issue of oilshell, which I solved with the sed s|^(.+) =~ ([^\$].+) ]]|regexp='\2'; \1 =~ $regexp ]]| that I based in a commit from you in 2021.

After that I had to deal with a Can't resolve dynamic argument in 'source' -- that's because airgeddon sources its scripts with source "${scriptfolder}${rc_file_name}" and well, I don't know how to correctly solve these even now 🙈 -- Should I replace this with relative-hardcoded-paths, I know that I can't use absolute-hardcoded-paths -- Kokada pointed me to "keep.source" but this didn't help, "fix" somehow did the trick for the "strings" and "known_pins", and for the "plugins" in a little hacky way. But I had to leave ".airgeddonrc" out of this and use "keep" on it. (very :lost: even now)

At this point, I had already taken a look at the resholve docs. And well, I've started to add all the deps that weren't documented and resholve was finding 🏆 . That lead me to these issues in the way:

  • "There is not yet a good way to resolve 'ping' in Nix builds." (Which don't come up with any suggestion, I've added it as an external)

  • "timeout" had a "15" where the lore was expecting an "executable argument" (No clue how to say it to skip only this parameter, so I changed it to "cannot", which I'm pretty sure I should not)

  • you repeat "execer" twice here:

    -- should be "wrapper" right? (This repeats in that README)

  • "tmux" is executing its arguments, even worse, airgeddon uses "send-keys" to throw commands in it. (What should I do here? Again I went with "cannot")

  • I had to come up with a hacky solution to keep optional dependencies optional, using the "external".

In the end, it ran, but the scripts in heredocs inside it were not using absolute references. They were even using the unpatched shebang. Airgeddon starts running one of these inside tmux and that wasn't finding the executables.

Originally posted by @PedroHLC in NixOS/nixpkgs#173885 (comment)

@abathur
Copy link
Owner Author

abathur commented May 26, 2022

Preserving this since there are at least a few things in here that need to get individually fixed ~ASAP.

@PedroHLC
Copy link

PedroHLC commented May 26, 2022

🤔 what if, for every fatal error, resholve added a link to the wiki in the message, as shellcheck does.

There is not yet a good way to resolve 'ping' in Nix builds.

See more: https://github.com/abathur/resholve/wiki/RS00042

Like: https://github.com/koalaman/shellcheck/wiki/SC1118 (Range SC1000~SC3060)

It could even link to a blank page, GitHub allows you to toggle the option to "Restrict editing to collaborators only".

@abathur
Copy link
Owner Author

abathur commented May 26, 2022

That (or something similar) is the long-term intent. There are a few boxes I'd like to check there...

  • provide specific recommendations at error time since they're a good reminder of what to do
  • probably link out to some other resource
  • drive the documentation and code from a single source

I tend towards over-thinking, and error messages and systems of appropriate codes are both things I can easily waste a lot of time what-ifing about.

I tried to sneak up on this about a year ago, as you can see in this diff, but I was spinning my wheels on specifics and decided to just freeze that effort for a bit (I think it makes sense to privilege features over docs in the short run, and at the time I was still trying to land all of this "lore" work).

This fall/winter I sorted out the basics of the documentation single-sourcing part, and started modularizing resholve's code a bit (to make it easier to generate the exceptions).

I guess that's all to say that the time is close to right. Probably a this-year sort of thing.

@abathur abathur changed the title I'll give you a step-for-step introduction of what I did and what I found, maybe this can help you to help me: notes from nixpkgs#173885 May 27, 2022
@abathur
Copy link
Owner Author

abathur commented May 28, 2022

I'm making 2 posts addressing this. The first focuses on things you noted that I've fixed today. The 2nd will just record some thoughts on the open questions...


That lead me to these issues in the way:

  • "timeout" had a "15" where the lore was expecting an "executable argument" (No clue how to say it to skip only this parameter, so I changed it to "cannot", which I'm pretty sure I should not)

Correct. You found a bug in resholve's parser for timeout. For illustration I addressed it in separate test/fix commits:

Good spot. I fixed it in 99cd196.

Both of these will need to get released and propagated to nixpkgs; not sure how quickly I'll cut a release.

@abathur
Copy link
Owner Author

abathur commented May 28, 2022

  • "There is not yet a good way to resolve 'ping' in Nix builds." (Which don't come up with any suggestion, I've added it as an external)

Yes. Sorry that one's a bit unhelpful, but I'm intentionally making this a little disruptive. I'm hoping to make more people aware of the underlying problem to build pressure for trying to find a fix up at the Nix/nixpkgs layer.

After that I had to deal with a Can't resolve dynamic argument in 'source' -- that's because airgeddon sources its scripts with source "${scriptfolder}${rc_file_name}" and well, I don't know how to correctly solve these even now 🙈 -- Should I replace this with relative-hardcoded-paths, I know that I can't use absolute-hardcoded-paths -- Kokada pointed me to "keep.source" but this didn't help, "fix" somehow did the trick for the "strings" and "known_pins", and for the "plugins" in a little hacky way. But I had to leave ".airgeddonrc" out of this and use "keep" on it. (very :lost: even now)

I noticed this one as well. I'm not quite sure what the "right" fix will be. The ideal way to do it would be something like:

  fix = {
    # ...
    "$scriptfolder" = [ "lib/airgeddon/" ];
    "$language_strings_file" = [ "language_strings.sh" ];
    "$known_pins_dbfile" = [ "known_pins.db" ];
    # ...
  };

But...

  1. The airgeddonrc being in share instead of lib causes trouble with this. I tried using an empty string for scriptfolder, but IIRC that broke the Nix API. I started considering a kludge like setting rc_file_name to "../share/airgeddon/.airgeddonrc".
  2. I decided to stop fiddling with it because I wasn't 100% sure if baking in the .airgeddonrc path was the right approach in the first place? Would it be fairly normal for users to modify it? Would a fair fraction of the people who use the package expect it to be picking up ~/.airgeddonrc or something?

But as I write this out, I wonder if this would work (on macOS atm, not easy to just test...):

  fix = {
    # ...
    "$scriptfolder" = [ "./" ];
    "$language_strings_file" = [ "lib/airgeddon/language_strings.sh" ];
    "$known_pins_dbfile" = [ "lib/airgeddon/known_pins.db" ];
    "$rc_file_name" = [ "share/airgeddon/.airgeddonrc" ];
    # ...
  };
  • "tmux" is executing its arguments, even worse, airgeddon uses "send-keys" to throw commands in it. (What should I do here? Again I went with "cannot")

I'm not a tmux user so I'm just kinda shooting from the hip here.

If the tmux arguments are very regular, it may just be a matter of creating a parser for it in resholve. Searching through the codebase for tmux invocations didn't make it terribly obvious what all commands are getting passed through it, since it seems like most of these are in utility functions.

But, I did see a lot of run-time code-generation behavior (such as https://github.com/v1s1t0r1sh3r3/airgeddon/blob/master/airgeddon.sh#L3732-L4142) that looks like the kind of thing I'd usually mark in my notes as as too dynamic for resholve to have much hope of handling.

This may still be fine if you don't actually need to resolve commands down in these dynamic structures (for example, if the entire thing is just using shell functions that do get resolved)? If they do hold external executable references, resholve will likely end up being a ~code-intel tool for you here, with the real solution needing to be some wrapping/patching combo?

resholve also has prologue and epilogue arguments--I suppose you could use that to also inject a PATH for the same dependencies at the top of the script. (But, of course, there's a risk these scripts hold additional unknown dependencies)

  • I had to come up with a hacky solution to keep optional dependencies optional, using the "external".

I liked what you came up with--I've been doing the same. I haven't quite decided if resholve needs a special lever for cases like this (there's a related case with scripts that will use the first found of N alternative programs for something--curl OR wget and is a common example) or just better documentation that fits the scenarios.

In the end, it ran, but the scripts in heredocs inside it were not using absolute references. They were even using the unpatched shebang. Airgeddon starts running one of these inside tmux and that wasn't finding the executables.

Aha. Hehe. Should've kept reading. Yes--the heredocs aren't really intelligible as Shell to resholve. If a general wrapper approach doesn't work for some reason, perhaps patching it to inject a generated PATH after the #!/usr/bin/env bash lines would.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants