[00:13] OvenWerks: teward is reviewing your code for ubuntustudio-menu-add. [00:13] cursory review [00:14] nothing in-depth ;) [00:14] Eickmeyer: OvenWerks: BUGS == ROADMAP, is this intentional? [00:15] (same file twice) [00:15] just curious [00:15] teward: I believe the file was supposed to be renamed ROADMAP, but the BUGS file didn't get deleted? Might've been a git collision, for lack of a better term. [00:15] nope still present there [00:15] smh [00:15] I can fix unless you want to just kill it with fire. [00:16] Eickmeyer: i don't have upload to this code branch [00:16] if you can update it in the git that'd be great [00:16] I can. standby... [00:16] your changelog also is UNRELEASED [00:16] which is not a valid target [00:16] I assume you want me to just alter that [00:16] Ugh... also will fix. [00:17] eesh, native package. [00:17] Not complaining MUCH but... [00:17] I prefer quilts ;) [00:17] not an impact on the review though just me shivering because Debianisms [00:17] and some chaos i've had to deal with there [00:17] Yeah, it's a native package. No reason to not be native since it's for Ubuntu only. [00:17] yep [00:17] Eickmeyer: other than those issues I pointed at [00:18] everything LOOKS OK [00:18] it'll need AA review of course, but fix the things I indicated [00:18] Ok. Fixing now. [00:18] for ubuntustudio-menu-add anyways [00:23] teward: Fixed. [00:30] Eickmeyer: ack. Going to run this through local sbuild with LIntian calls, as well [00:30] give me a bit [00:30] gotta spin an eoan chroot ;) [00:30] No worries. [00:33] hmmmmmmmmmmmmm [00:37] Eickmeyer: there's a few things in the built binaries that throw some lintian pedantic warnings [00:37] W: ubuntustudio-menu-add: binary-without-manpage usr/bin/ubuntustudio-menu-add [00:38] if this shouldn't have a manpage you might want an override... [00:38] and this is when `lintian --pedantic` is run on the built binaries inside the sbuild schroot [00:39] it passes but not sure how pedantic you want me to be with warnings [00:40] teward: In the past, vorlon, infinity, and sil2100 have just let that one go. [00:40] ack [00:40] i'll make a note of it but i'll consider it 'passing' [00:40] since it didnt lintian-fail [00:40] just something i noticed [00:40] Ok. [00:40] ack [00:40] and i'm overly pedantic ;) [00:40] and in Debian they whine more when that triggers [00:41] Yeah. [00:41] builds clean, looks lintian clean except for that [00:42] Cool. [00:43] teward: When (if) you get to dpf-plugins, don't expect manpages for that either. Those plugins are something that you would only run if you already knew how to run them. [00:43] Live/Recording Audio Engineers, for example. [00:45] yeah i'm just overly pedantic from the Debianisms I've had to handle :P [00:45] No worries. [00:52] Eickmeyer: uploaded to NEW queue [00:53] * teward needs food [00:53] teward: Saw that, thanks! [01:01] Eickmeyer: eoan-proposed next time though the system redirects it [01:02] teward: I was always told to target the release directly, letting the system redirect it. [01:02] *shrugs* [01:02] i'm picky ;) [01:02] but all's good [01:02] the system'll redirect it regardless :) [01:02] * Eickmeyer *thumbsup* [01:04] OvenWerks: what Eickmeyer is NOT telling you is I want to school you in how PEP8 works :P [01:04] and how triple-apostrophe strings are NOT comments. [01:04] flake threw all sorts of "WTH" info level warns there :| [01:06] Eickmeyer: OvenWerks: you do NOT want to know the complaints sarnold has [01:08] ahoy pals, could I beg someone to replace all the calls to rm and cp with native python calls instead, in https://git.launchpad.net/ubuntustudio-menu-add/tree/usr/bin/ubuntustudio-menu-add ? [01:08] sarnold: you should NACK it [01:09] it'd also be a huge favour to me to rexplace the exoedit thing with an array-based execution, see https://docs.python.org/3/library/subprocess.html#subprocess.run for an example [01:10] Oh boy... [01:10] https://docs.python.org/3/library/os.html#os.unlink [01:10] the file *copy* is annoying, but this may be a suitable replacement: https://docs.python.org/3/library/shutil.html#shutil.copyfile [01:11] sarnold: https://bugs.launchpad.net/ubuntustudio-menu-add/+bug/1831154 also please [01:11] Launchpad bug 1831154 in Ubuntu Studio Menu Add "[needs-packaging] ubuntustudio-menu-add" [Medium,In progress] [01:11] i would suggest you also offload your comments there [01:11] Eickmeyer: OvenWerks: I also ran a "dig" into here with my own knowledge and though I don't see an obvious exploit mechanism, I *HATE* subprocess wrappers where not needed [01:11] esp. in latest Python in Eoan [01:12] so I asked AA / release team to REJECT the package as-is [01:12] as well, all the things sarnold indicated [01:12] ack [01:13] i didn't do a full code dig and though I've DONE these kinds of subprocess wraps before... [01:13] there's inbuilt os calls for lots of these things [01:13] and those're preferred to these subprocess wraps [01:13] ALTERNATIVELY [01:13] give me direct upload rights to your code base [01:13] i'll rewrite it myself ;) [01:13] *shot* [01:15] oh wait [01:15] i forgot [01:15] my membership gives me upload :| [01:15] teward: Haha [01:15] it's still polite to ask :) [01:15] teward: Go ahead. I trust you. [01:15] i'll provide a patch for it that they can apply :p [01:15] sarnold: and let you review it too xD [01:19] UNRELATED also use spaces not tabs [01:19] because tabs suck [01:23] this... is messy :| [01:24] sarnold: we're in Py 3.7+ in Eoan right? [01:25] teward: I think so; at the moment umt search reports python 3.7 in main, 3.8 in universe in eoan [01:25] 3.7+ is fine [01:26] because formatstrings [01:26] saves doing string arithmetic [01:27] teward, sarnold: OvenWerks does most of his development in bionic, I believe. [01:28] Eickmeyer: they can still use formatstrings :P [01:28] the point is that i hate this string arithmetic [01:28] "string data1:{} data2:{}".format(value1, value2) [01:28] I know, I was just referring to the 3.7 vs 3.8. [01:28] ^ condensed to: f'string data1:{value1} data2:{value2}' [01:29] * Eickmeyer couldn't code his way out of a paper bag, so he sticks to packaging [01:29] but as long as it's 3.7+ it's fine [01:29] note to self: don't code review without food [01:36] Eickmeyer: you and OvenWerks are going to hate me [01:36] because of this patch [01:36] teward: I'm not, and I hope OvenWerks doesn't. [01:37] I am just grateful for the contribution! [01:38] oh this is a LARGE patch [01:39] sarnold: oh fun fact [01:39] your subprocess array approach? [01:39] for the one remaining SP call? [01:39] sp.run(shlex.split("the full command string"), ...) <-- shortcut [01:40] i know a few tricks LOL [01:40] teward: btw I hate that approach [01:41] the whole point of using the array based approach is that the programmer actually knows what the arguments are, and asking the shell, or shlex, or whatever, to split them apart, is likely to break something, or let some smartasss name a file $(id) or whatever [01:43] sarnold: true. [01:43] Look who's in the Ubuntu Studio chat :D [01:43] sarnold: but in this case these args don't seem fixed that way. We could do that though [01:43] @tsimonq2 there's a nuke with your name on it waiting in the Lubuntu infra [01:43] go fix it [01:44] @teward [ @tsimonq2 there's a nuke with your name on it waiting in the Lubuntu in …], Oh? [01:46] whoopsies i forgot to attach that patch [01:46] Eickmeyer: sarnold: patch attached to the bug [01:46] sarnold: i think you'll be happy with it [01:46] but the patch is HUGE because tabs -> 4-spaces [01:46] because the other thing I found was ***mixed punctuation*** [01:46] s/punctuation/indentations/ [01:46] which throws INdentationErrors [01:47] sarnold: this patch is not a small one, but it pulls all the subprocess wrapped rm/cp and replaces them with os.unlink/shutil.copyfile accordingly [01:49] tsimonq2: and no there's no nuke in the infra for you to deal with [01:49] i just wanted to drive you insane :P [01:49] Eickmeyer has an annoying habit they picked up from me [01:49] being insistent on asking for things [01:49] like sponsoring [01:49] ... or in this case NOT tearing OvenWerks a new one [01:51] Eickmeyer: wrt developing in Bionic... [01:51] teward: I appreciate it. [01:51] i dev for 3.7/3.8 in Bionic too [01:51] pyenv is my friend [01:51] also lets me have 3.5 for the ancient infra I have to support for some older boxes [01:52] Eickmeyer: this'll need a run test of course [01:52] not just a build test [01:52] teward: ack [01:52] because I had to remove some of the logic that WAS in place because subprocess is gone for some calls [01:52] the logic *SHOULD* still function as is [01:52] but it's... eh [01:53] teward: it's usually better to stuff whitespace patches into their own thing, so the substantive fixes are easier to review [01:53] good point [01:53] sarnold: i can split that out but in this case my IDE did the autochanging [01:54] sarnold: still, i think you'll like this. https://paste.ubuntu.com/p/JX8gBK6fM4/ has the code after my revisions [01:55] ahhhh that's the stuff :) [02:26] * OvenWerks actually has his tabs set to 2 spaces width [02:28] teward: thank you for your work. I am relatively new to python, coming from c,c++,have done perl and tcl/tk in the past as well [02:29] my main reason for working in python at all it that it seems to be favoured and is less likey to require adding more libs (though tcl/tk is used by enough other stuff it is always there for the studio iso) [02:32] OvenWerks: There's a patch in the bug report if you want to apply it and play with it. [02:33] I was looking for that... thanks. [02:34] I can create a man page if needed, but it would just be a rehash of the included help in the GUI :) [02:34] sorry I forgot. [02:34] bug 1831154 [02:34] bug 1831154 in Ubuntu Studio Menu Add "[needs-packaging] ubuntustudio-menu-add" [Medium,In progress] https://launchpad.net/bugs/1831154 [02:35] OvenWerks: A man page is always welcome and would get rid of that lintian warning. [02:35] teward: ^ [02:36] with all the tab to space fixes I can't see what he did... [02:38] OvenWerks: It's mostly whitespaces. I'd look at the diff if you can. [02:39] unlink is fine though. At some point I should change the var cmd to something more descriptive [02:48] teward: I will try to remember that for python I should for some insane reason use spaces instead of tabs. :P [02:49] I am actually surprised there were any spaces [02:49] must have been cut and paste [02:52] teward: I have tried using array based execution in the past and after loosing some hair went to the define comandline, run. Leftovers from bash no doubt and much easier to read from my POV. [02:52] anyway. I will run the patch. [03:10] Ga!, now it doesn't work... [03:40] Ya, 0k... [03:52] teward: same problem... array based execution doesn't work. [05:02] teward: I have put back in the string based command line and uploaded. I have yet to be able to get array based commands to work (I have also tried in the past) and in fact I have had problems using a joined string within the command as well... which is why a define the command line first as a string and use it. That is all that has ever actually worked [05:03] Eickmeyer: ^^ [05:05] teward and sarnold are more worried about the security implications for this. AFAIK, they’re both east-coasters, so they’ll see this in the morning. [06:05] Eickmeyer: maybe they can figure out why the array based execution doesn't work. [12:31] Probably a subproccess headache [12:31] which I run into myself [12:31] sarnold: ^ this is usually why I hate subprocess calls. [12:32] OvenWerks: I can fix that as well - but for some reason there was mixed indentation. Reverting the array based call is an easy you can do yourself but the main problem was the unsafe calls to cp/rm via subprocess [12:33] because that can easily be exploited with the right few code stabs [12:33] though exploitability on that is LOW it's still a headache [12:33] let me poke at this after I get to work [13:46] sarnold: call/run don't seem to accept things properly, so i'm going to go with their initial approach which I think works. [13:46] there's some oddness in subprocess with how commands are passed and I REGULARLY have this headache. [13:52] OvenWerks: I condensed the 'old way' you were doing into a single line - https://git.launchpad.net/ubuntustudio-menu-add/commit/?id=f395294eb0dc1efd24be846e27974b453c5e80ff [13:52] that's the ONLY time i'm pushing to the code [13:53] if you and Eickmeyer / Eickmeyer[m] would like to test this that'd be great. [13:53] i'll make a note in the bug [13:53] if it works with that in line then that should take care of the major issues [13:53] and i'll do another last-minute review [13:54] * teward001 pours some salt on @Eickmeyer just to make sure they're pinged accordingly. [14:37] teward: Did you commit it to master? [14:38] Nm, I see that you did. [14:56] Eickmeyer: yes? [14:56] I SAID nevermind. ;) [14:56] :P [14:56] *salts Eickmeyer anyways* [14:57] * Eickmeyer throws a full coffee mug at teward [14:57] * teward already has had 4 coffees, does not need another [15:04] teward: cp/rm was just me not learning python well enough. The subprocess.call string formating is different and so probably works. I have always tried putting the string together with + and that did not work for me. [15:05] I struggled with the subprocess calls for a long time before I came up with the way I finally used [15:05] OvenWerks: i actually tested both run AND call [15:05] with an 'echo' command [15:05] only worked with strings [15:05] which means I'll go digging on that later [15:05] I COULD HAVE SWORN I got it to work [15:05] but i'll have to go poking at my other scripts [15:06] OvenWerks: and I have no provlem with the subprocess calls :) [15:06] the only difference is [15:06] instead of var = somestr; sp.call(var, ...) [15:06] just define the string inside the call ;) [15:06] that's the only one-line condensing I did :) [15:06] HOLY HANNA I'm spamming myself on AndroIRC xD [15:07] Yes that works just fine. [15:07] yep. And saves some memory space, albeit only a little bit :) [15:08] I also removed some excess comments last night. [15:09] teward: I had never seen that method of creating strings before. Is there a quick link I can use to read up on it? [15:09] (and does it work inside of a print() as well?) [15:10] https://realpython.com/python-f-strings/ [15:10] does it work on the right side of an = [15:10] OvenWerks: yes, it works wherever a string would be defined [15:10] it's actually born from "".format [15:10] which https://www.geeksforgeeks.org/python-format-function/ is decent at explaining [15:11] OvenWerks: but the f"foo {blah}" where blah is a variable format is 'new' enough I don't trust it on < py3.7 [15:11] cool, it may make things better in -controls too then. (no there are no cp/rm in there) [15:11] it's supported in 3.6 but :) [15:11] OvenWerks: indeed, there's a ton of ways to condense [15:11] to quote sarnold, I could probably if I put my mind to it make things a lot shorter [15:11] but :P [15:11] I tend to backwards-compat my code for 16.04's py3.5 so I use "{}".format(var) instead [15:12] teward: I learn by doing. [15:12] but i've been falling in LOVE with the in-line strings :) [15:12] yep [15:12] well that's how you learn :) [15:12] I learn by doing as well [15:12] but i don't have any RECENT code examples on it :) [15:12] keep in mind also i only JUST got coredev so my bootprints are not yet everywhere :p [15:12] they're starting to show up though, with this package :P [15:13] mmmmm, dark-chocolate covered peanuts... *snacks* [15:13] coffee [15:13] had that [15:13] 4 cups already :) [15:13] but caffeine doesn't negate hunger so [15:13] I just got up and made breakfast for my son. [15:13] indeed :) [15:14] my eating will wait till my Yf gets up. [15:14] 11:14 here so by now i should probably switch into 'lunch' mode soon enough heh [15:15] Food is food, I still call meat cow and pig rather than beef and pork. [15:19] f-strings "new"? actually it is kinds like the old school (bash, perl) ${var} [15:21] Holy crap, the build far is either stuck or extremely busy. 17050 jobs in the queue, 23 hour wait. [15:21] *farm [15:33] OvenWerks: new for Python ;) [15:33] Eickmeyer: yikes, there was build issues earlier [15:34] @teward001: cjwatson said they were doing a test rebuild. [15:35] that would explain it [16:34] Eickmeyer: if you need me to fast-build I can compile for amd64 for you to test sooner [16:34] but you'll of course have to download the .deb and install [16:35] teward: Yeah, I coul download the .deb now and do it, tbh. It's done building, it just hasn't published to the PPA. Looks like build dispatching is also paused, so publishing is likely not to happen as well. [16:36] i'll sbuild it on eoan amd64 local then upload somewhere standby [16:37] I stand corrected, it just published. [16:37] * Eickmeyer is installing the built deb now [16:38] heh [16:38] well that was a fast build now xD [16:39] Eickmeyer: https://people.ubuntu.com/~teward/ubuntustudio-menu-add_0.1_all.deb if the PPA is slow :P [16:39] yay for sbuild :) [16:40] teward: Already have it. Everything works from my tests, but OvenWerks might have some more in-depth testing. [16:40] OvenWerks: New build in autobuilds, you can probably just apt upgrade. [16:41] and if that doesn't work for you I have the built binary in that link above :) [16:41] Eickmeyer: I almost totally forgot about 'people.u.c' though as an SFTP upload point xD [16:41] * Eickmeyer hasn't used people.u.c, and probably could. [17:44] OvenWerks: Let us (me & teward) know how testing ubuntustudio-menu-add goes. [17:44] * teward001 was poked [17:44] * teward001 goes back to poking redis-server instead [17:44] hehe [17:45] * teward drops a crate of onions on Eickmeyer [17:45] oops :) [17:45] MMmmm... walla walla sweets... [17:47] * Eickmeyer starts grilling the onions for burgers [17:47] * sarnold spills some kosher salt and slices off a big piece of onion [17:48] ^this guy onions. [17:49] i'm sorry you assume that sarnold is NOT ALREADY an onion :p [17:49] *shot* [17:50] Eickmeyer: it may be a fewhours before I can play with it. The machine I am on is still 16.04... (son's laptop) [17:50] OvenWerks: No worries. teward^ [17:51] * Eickmeyer wanders off to play a game with his son [17:54] ack [18:17] OvenWerks: i miiight have a way to make the 'subprocess.run' with an array work [18:17] but we need to drop shell=True possibly [18:17] (to sate sarnold's concerns) [18:24] OvenWerks: Eickmeyer: specifically, http://people.ubuntu.com/~teward/array-based_subprocess_execution_of_exo-desktop-item-edit.patch [18:24] which i THINK works proper [18:24] but again, needs the tests :) [18:24] no rush [18:27] Eickmeyer: OvenWerks: http://people.ubuntu.com/~teward/ubuntustudio-menu-add_0.1_all_v2.deb <-- with that last patch [18:27] in case you want to test :) [18:28] (since the builders are slow) [18:28] teward: I think the terminal was there because it uses the binary name without the path. [18:28] possibly, but I don't think hwe have a choice so longa s the binary is in PATH [18:28] again one of those things I tried more than one way [18:28] OvenWerks: we could also hardcode the full path [18:29] which is probably the even safer approach [18:29] i don't know the full path though for that binary [18:29] I don't suppose it is likely to move from one cycle to the next ;) [18:29] yeah, in which case hardcode it [18:29] i know that hardcoding things like that *tends* to be a bad thing [18:29] but we can always patch code later if things cahnge :P [18:29] $ which exo-desktop-item-edit [18:30] /usr/bin/exo-desktop-item-edit [18:30] change* [18:31] patch updated i'll update that .deb :p [18:31] using the full path is always considered more secure [18:31] yes, it is [18:32] pk won't allow anything else [18:33] http://people.ubuntu.com/~teward/ubuntustudio-menu-add_0.1_all_v3.deb is the 3rd deb version then including the fullpath this time [18:33] again, in case you all want to supersede the speed of the builders ;) [18:34] teward: I won't be able to test for a few hours (prolly 1430 local (-0700)) [18:34] that's fine [18:34] as i said no rush [18:34] but the builders are stressed right now [18:34] so :P [18:35] Spending time with my Yf before she goes to work. [18:35] no problem :)