ananicy-cpp: support wrapped applications #319634
Conversation
|
I can't find how to request reviews so: @JohnRTitor @Artturin |
| + if (utils::regex::is_str_matching_regex(name, "^\\..+-wrap(p(ed?)?)?$")) | ||
| + name = name.substr(1, name.find_last_of('-') - 1); |
There was a problem hiding this comment.
So it matches -wrapped and then removes the -wrapped right?
What's up with the (p(ed?)?)?
There was a problem hiding this comment.
AFAIK, sometimes process names don't fully have "-wrapped" suffix in their name, due to size limitations.
There was a problem hiding this comment.
It matches
.x-wrap
.x-wrapp
.x-wrappe
.x-wrapped
and removes everything after the last - (the - included).
I choose that regex because I have all of those on my system, and I doubt there are any other patterns
There was a problem hiding this comment.
Not all process names end like that, as you can see.
I had to manually add these rules to match .x- .x-w, .x-wra and so on.
All wrapped app's processes have . in the beginning.
I think the rules should run on any process x and \.x.* (regex)
There was a problem hiding this comment.
@JohnRTitor are you sure that screenshots includes the full process names?
There was a problem hiding this comment.
Looks like they are, in which case I'll try to come up with a better way of stripping the extra characters. I'm not sure how yet, so ideas are welcome.
There was a problem hiding this comment.
Never mind, it looks like ps is doing something funky. If you check the nixpkgs makeWrapper implementation, you can see that wrapers always start with a . and end with -wrapped. You can also confirm this by check the store paths for programs, eg.: ls -a $(dirname $(realpath $(which firefox))). And fortunately the process names in ananicy-cpp are also correct, which I confirmed by logging:
(The right side is ps, the left side is a log line from ananicy-cpp)
diniamo
force-pushed
the
ananicy-cpp-patch-for-wrappers
branch
from
c1077a8
to
d45e0f1
Compare
diniamo
force-pushed
the
ananicy-cpp-patch-for-wrappers
branch
from
d45e0f1
to
3565fd8
Compare
|
@JohnRTitor @Artturin could we finish this up? |
|
Is there anything need to do to merge this? |
Looks good, let me do a quick check before I approve. |
| + if (name.starts_with('.') && name.ends_with("-wrapped")) | ||
| + name = name.substr(1, name.find_last_of('-') - 1); | ||
| + const auto &rule = rules->get_rule(name); |
There was a problem hiding this comment.
Does the . have to be removed?
There was a problem hiding this comment.
I think . at the beginning should be removed, because the rules don't necessarily conform to .process-wrapped but rather process.
There was a problem hiding this comment.
Yes, it does. Both the . and the -wrapped are appended by the wrapper script, so we want the part between them for matching.
There was a problem hiding this comment.
I meant is the . being removed, I guess the (1 is doing that? Not familiar with cpp
|
By the way, is there a reason we are not using |
Good point, open a pr for it I think |
|
@JohnRTitor can you make a pr to remove all the now reduant rules in the cachyos rules repo? |
|
I think those can stay, for those using just Future rules to the repo, won't need to account for wrapped processes. |
Description of changes
Ananicy-cpp now matches wrapped applications with this patch.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 馃憤 reaction to pull requests you find important.