/srv/irclogs.ubuntu.com/2019/06/19/#ubuntustudio-devel.txt

EickmeyerOvenWerks: teward is reviewing your code for ubuntustudio-menu-add.00:13
tewardcursory review00:13
tewardnothing in-depth ;)00:14
tewardEickmeyer: OvenWerks: BUGS == ROADMAP, is this intentional?00:14
teward(same file twice)00:15
tewardjust curious00:15
Eickmeyerteward: 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
tewardnope still present there00:15
Eickmeyersmh00:15
EickmeyerI can fix unless you want to just kill it with fire.00:15
tewardEickmeyer: i don't have upload to this code branch00:16
tewardif you can update it in the git that'd be great00:16
EickmeyerI can. standby...00:16
tewardyour changelog also is UNRELEASED00:16
tewardwhich is not a valid target00:16
tewardI assume you want me to just alter that00:16
EickmeyerUgh... also will fix.00:16
tewardeesh, native package.00:17
tewardNot complaining MUCH but...00:17
tewardI prefer quilts ;)00:17
tewardnot an impact on the review though just me shivering because Debianisms00:17
tewardand some chaos i've had to deal with there00:17
EickmeyerYeah, it's a native package. No reason to not be native since it's for Ubuntu only.00:17
tewardyep00:17
tewardEickmeyer: other than those issues I pointed at00:17
tewardeverything LOOKS OK00:18
tewardit'll need AA review of course, but fix the things I indicated00:18
EickmeyerOk. Fixing now.00:18
tewardfor ubuntustudio-menu-add anyways00:18
Eickmeyerteward: Fixed.00:23
tewardEickmeyer: ack.  Going to run this through local sbuild with LIntian calls, as well00:30
tewardgive me a bit00:30
tewardgotta spin an eoan chroot ;)00:30
EickmeyerNo worries.00:30
tewardhmmmmmmmmmmmmm00:33
tewardEickmeyer: there's a few things in the built binaries that throw some lintian pedantic warnings00:37
tewardW: ubuntustudio-menu-add: binary-without-manpage usr/bin/ubuntustudio-menu-add00:37
tewardif this shouldn't have a manpage you might want an override...00:38
tewardand this is when `lintian --pedantic` is run on the built binaries inside the sbuild schroot00:38
tewardit passes but not sure how pedantic you want me to be with warnings00:39
Eickmeyerteward: In the past, vorlon, infinity, and sil2100 have just let that one go.00:40
tewardack00:40
tewardi'll make a note of it but i'll consider it 'passing'00:40
tewardsince it didnt lintian-fail00:40
tewardjust something i noticed00:40
EickmeyerOk.00:40
Eickmeyerack00:40
tewardand i'm overly pedantic ;)00:40
tewardand in Debian they whine more when that triggers00:40
EickmeyerYeah.00:41
tewardbuilds clean, looks lintian clean except for that00:41
EickmeyerCool.00:42
Eickmeyerteward: 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
EickmeyerLive/Recording Audio Engineers, for example.00:43
tewardyeah i'm just overly pedantic from the Debianisms I've had to handle :P00:45
EickmeyerNo worries.00:45
tewardEickmeyer: uploaded to NEW queue00:52
* teward needs food00:53
Eickmeyerteward: Saw that, thanks!00:53
tewardEickmeyer: eoan-proposed next time though the system redirects it01:01
Eickmeyerteward: I was always told to target the release directly, letting the system redirect it.01:02
teward*shrugs*01:02
tewardi'm picky ;)01:02
tewardbut all's good01:02
tewardthe system'll redirect it regardless :)01:02
* Eickmeyer *thumbsup*01:02
tewardOvenWerks: what Eickmeyer is NOT telling you is I want to school you in how PEP8 works :P01:04
tewardand how triple-apostrophe strings are NOT comments.01:04
tewardflake threw all sorts of "WTH" info level warns there :|01:04
tewardEickmeyer: OvenWerks: you do NOT want to know the complaints sarnold has01:06
sarnoldahoy 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
tewardsarnold: you should NACK it01:08
sarnoldit'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 example01:09
EickmeyerOh boy...01:10
sarnoldhttps://docs.python.org/3/library/os.html#os.unlink01:10
sarnoldthe file *copy* is annoying, but this may be a suitable replacement: https://docs.python.org/3/library/shutil.html#shutil.copyfile01:10
tewardsarnold: https://bugs.launchpad.net/ubuntustudio-menu-add/+bug/1831154 also please01:11
ubottuLaunchpad bug 1831154 in Ubuntu Studio Menu Add "[needs-packaging] ubuntustudio-menu-add" [Medium,In progress]01:11
tewardi would suggest you also offload your comments there01:11
tewardEickmeyer: 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 needed01:11
tewardesp. in latest Python in Eoan01:11
tewardso I asked AA / release team to REJECT the package as-is01:12
tewardas well, all the things sarnold indicated01:12
Eickmeyerack01:12
tewardi didn't do a full code dig and though I've DONE these kinds of subprocess wraps before...01:13
tewardthere's inbuilt os calls for lots of these things01:13
tewardand those're preferred to these subprocess wraps01:13
tewardALTERNATIVELY01:13
tewardgive me direct upload rights to your code base01:13
tewardi'll rewrite it myself ;)01:13
teward*shot*01:13
tewardoh wait01:15
tewardi forgot01:15
tewardmy membership gives me upload :|01:15
Eickmeyerteward: Haha01:15
sarnoldit's still polite to ask :)01:15
Eickmeyerteward: Go ahead. I trust you.01:15
tewardi'll provide a patch for it that they can apply :p01:15
tewardsarnold: and let you review it too xD01:15
tewardUNRELATED also use spaces not tabs01:19
tewardbecause tabs suck01:19
tewardthis... is messy :|01:23
tewardsarnold: we're in Py 3.7+ in Eoan right?01:24
sarnoldteward: I think so; at the moment umt search reports python 3.7 in main, 3.8 in universe in eoan01:25
teward3.7+ is fine01:25
tewardbecause formatstrings01:26
tewardsaves doing string arithmetic01:26
Eickmeyerteward, sarnold: OvenWerks does most of his development in bionic, I believe.01:27
tewardEickmeyer: they can still use formatstrings :P01:28
tewardthe point is that i hate this string arithmetic01:28
teward"string data1:{} data2:{}".format(value1, value2)01:28
EickmeyerI know, I was just referring to the 3.7 vs 3.8.01:28
teward^ condensed to: f'string data1:{value1} data2:{value2}'01:28
* Eickmeyer couldn't code his way out of a paper bag, so he sticks to packaging01:29
tewardbut as long as it's 3.7+ it's fine01:29
tewardnote to self: don't code review without food01:29
tewardEickmeyer: you and OvenWerks are going to hate me01:36
tewardbecause of this patch01:36
Eickmeyerteward: I'm not, and I hope OvenWerks doesn't.01:36
EickmeyerI am just grateful for the contribution!01:37
tewardoh this is a LARGE patch01:38
tewardsarnold: oh fun fact01:39
tewardyour subprocess array approach?01:39
tewardfor the one remaining SP call?01:39
tewardsp.run(shlex.split("the full command string"), ...)  <-- shortcut01:39
tewardi know a few tricks LOL01:40
sarnoldteward: btw I hate that approach01:40
sarnoldthe 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 whatever01:41
tewardsarnold: true.01:43
studiobot<tsimonq2> Look who's in the Ubuntu Studio chat :D01:43
tewardsarnold: but in this case these args don't seem fixed that way.  We could do that though01:43
teward@tsimonq2 there's a nuke with your name on it waiting in the Lubuntu infra01:43
tewardgo fix it01:43
studiobot<tsimonq2> @teward [<teward> @tsimonq2 there's a nuke with your name on it waiting in the Lubuntu in …], Oh?01:44
tewardwhoopsies i forgot to attach that patch01:46
tewardEickmeyer: sarnold: patch attached to the bug01:46
tewardsarnold: i think you'll be happy with it01:46
tewardbut the patch is HUGE because tabs -> 4-spaces01:46
tewardbecause the other thing I found was ***mixed punctuation***01:46
tewards/punctuation/indentations/01:46
tewardwhich throws INdentationErrors01:46
tewardsarnold: this patch is not a small one, but it pulls all the subprocess wrapped rm/cp and replaces them with os.unlink/shutil.copyfile accordingly01:47
tewardtsimonq2: and no there's no nuke in the infra for you to deal with01:49
tewardi just wanted to drive you insane :P01:49
tewardEickmeyer has an annoying habit they picked up from me01:49
tewardbeing insistent on asking for things01:49
tewardlike sponsoring01:49
teward... or in this case NOT tearing OvenWerks a new one01:49
tewardEickmeyer: wrt developing in Bionic...01:51
Eickmeyerteward: I appreciate it.01:51
tewardi dev for 3.7/3.8 in Bionic too01:51
tewardpyenv is my friend01:51
tewardalso lets me have 3.5 for the ancient infra I have to support for some older boxes01:51
tewardEickmeyer: this'll need a run test of course01:52
tewardnot just a build test01:52
Eickmeyerteward: ack01:52
tewardbecause I had to remove some of the logic that WAS in place because subprocess is gone for some calls01:52
tewardthe logic *SHOULD* still function as is01:52
tewardbut it's... eh01:52
sarnoldteward: it's usually better to stuff whitespace patches into their own thing, so the substantive fixes are easier to review01:53
tewardgood point01:53
tewardsarnold: i can split that out but in this case my IDE did the autochanging01:53
tewardsarnold: still, i think you'll like this.  https://paste.ubuntu.com/p/JX8gBK6fM4/ has the code after my revisions01:54
sarnoldahhhh that's the stuff :)01:55
* OvenWerks actually has his tabs set to 2 spaces width02:26
OvenWerksteward: 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 well02:28
OvenWerksmy 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:29
EickmeyerOvenWerks: There's a patch in the bug report if you want to apply it and play with it.02:32
OvenWerksI was looking for that... thanks.02:33
OvenWerksI can create a man page if needed, but it would just be a rehash of the included help in the GUI :)02:34
OvenWerkssorry I forgot.02:34
Eickmeyerbug 183115402:34
ubottubug 1831154 in Ubuntu Studio Menu Add "[needs-packaging] ubuntustudio-menu-add" [Medium,In progress] https://launchpad.net/bugs/183115402:34
EickmeyerOvenWerks: A man page is always welcome and would get rid of that lintian warning.02:35
Eickmeyerteward: ^02:35
OvenWerkswith all the tab to space fixes I can't see what he did...02:36
EickmeyerOvenWerks: It's mostly whitespaces. I'd look at the diff if you can.02:38
OvenWerksunlink is fine though. At some point I should change the var cmd to something more descriptive02:39
OvenWerksteward: I will try to remember that for python I should for some insane reason use spaces instead of tabs. :P02:48
OvenWerksI am actually surprised there were any spaces02:49
OvenWerksmust have been cut and paste02:49
OvenWerksteward: 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
OvenWerksanyway. I will run the patch.02:52
OvenWerksGa!, now it doesn't work...03:10
OvenWerksYa, 0k...03:40
OvenWerksteward: same problem... array based execution doesn't work.03:52
OvenWerksteward: 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 worked05:02
OvenWerksEickmeyer: ^^05:03
Eickmeyer[m]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.05:05
OvenWerksEickmeyer: maybe they can figure out why the array based execution doesn't work.06:05
studiobot<teward001> Probably a subproccess headache12:31
studiobot<teward001> which I run into myself12:31
studiobot<teward001> sarnold: ^ this is usually why I hate subprocess calls.12:31
studiobot<teward001> 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 subprocess12:32
studiobot<teward001> because that can easily be exploited with the right few code stabs12:33
studiobot<teward001> though exploitability on that is LOW it's still a headache12:33
studiobot<teward001> let me poke at this after I get to work12:33
tewardsarnold: 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
tewardthere's some oddness in subprocess with how commands are passed and I REGULARLY have this headache.13:46
tewardOvenWerks: I condensed the 'old way' you were doing into a single line - https://git.launchpad.net/ubuntustudio-menu-add/commit/?id=f395294eb0dc1efd24be846e27974b453c5e80ff13:52
tewardthat's the ONLY time i'm pushing to the code13:52
tewardif you and Eickmeyer / Eickmeyer[m] would like to test this that'd be great.13:53
tewardi'll make a note in the bug13:53
tewardif it works with that in line then that should take care of the major issues13:53
tewardand i'll do another last-minute review13:53
studiobot* teward001 pours some salt on @Eickmeyer just to make sure they're pinged accordingly.13:54
Eickmeyerteward: Did you commit it to master?14:37
EickmeyerNm, I see that you did.14:38
tewardEickmeyer: yes?14:56
EickmeyerI SAID nevermind. ;) 14:56
teward:P14:56
teward*salts Eickmeyer anyways*14:56
* Eickmeyer throws a full coffee mug at teward14:57
* teward already has had 4 coffees, does not need another14:57
OvenWerksteward: 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:04
OvenWerksI struggled with the subprocess calls for a long time before I came up with the way I finally used15:05
tewardOvenWerks: i actually tested both run AND call15:05
tewardwith an 'echo' command15:05
tewardonly worked with strings15:05
tewardwhich means I'll go digging on that later15:05
tewardI COULD HAVE SWORN I got it to work15:05
tewardbut i'll have to go poking at my other scripts15:05
tewardOvenWerks: and I have no provlem with the subprocess calls :)15:06
tewardthe only difference is15:06
tewardinstead of var = somestr; sp.call(var, ...)15:06
tewardjust define the string inside the call ;)15:06
tewardthat's the only one-line condensing I did :)15:06
studiobot<teward001> HOLY HANNA I'm spamming myself on AndroIRC xD15:06
OvenWerksYes that works just fine.15:07
tewardyep.  And saves some memory space, albeit only a little bit :)15:07
OvenWerksI also removed some excess comments last night.15:08
OvenWerksteward: 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
OvenWerks(and does it work inside of a print() as well?)15:09
tewardhttps://realpython.com/python-f-strings/15:10
OvenWerksdoes it work on the right side of an =15:10
tewardOvenWerks: yes, it works wherever a string would be defined15:10
tewardit's actually born from "".format15:10
tewardwhich https://www.geeksforgeeks.org/python-format-function/ is decent at explaining15:10
tewardOvenWerks: but the f"foo {blah}" where blah is a variable format is 'new' enough I don't trust it on < py3.715:11
OvenWerkscool, it may make things better in -controls too then. (no there are no cp/rm in there)15:11
tewardit's supported in 3.6 but :)15:11
tewardOvenWerks: indeed, there's a ton of ways to condense15:11
tewardto quote sarnold, I could probably if I put my mind to it make things a lot shorter15:11
tewardbut :P15:11
tewardI tend to backwards-compat my code for 16.04's py3.5 so I use "{}".format(var) instead15:11
OvenWerksteward: I learn by doing.15:12
tewardbut i've been falling in LOVE with the in-line strings :)15:12
tewardyep15:12
tewardwell that's how you learn :)15:12
tewardI learn by doing as well15:12
tewardbut i don't have any RECENT code examples on it :)15:12
tewardkeep in mind also i only JUST got coredev so my bootprints are not yet everywhere :p15:12
tewardthey're starting to show up though, with this package :P15:12
tewardmmmmm, dark-chocolate covered peanuts... *snacks*15:13
OvenWerkscoffee15:13
tewardhad that15:13
teward4 cups already :)15:13
tewardbut caffeine doesn't negate hunger so15:13
OvenWerksI just got up and made breakfast for my son.15:13
tewardindeed :)15:13
OvenWerksmy eating will wait till my Yf gets up.15:14
teward11:14 here so by now i should probably switch into 'lunch' mode soon enough heh15:14
OvenWerksFood is food, I still call meat cow and pig rather than beef and pork.15:15
OvenWerksf-strings "new"? actually it is kinds like the old school (bash, perl) ${var}15:19
EickmeyerHoly crap, the build far is either stuck or extremely busy. 17050 jobs in the queue, 23 hour wait. 15:21
Eickmeyer*farm15:21
studiobot<teward001> OvenWerks: new for Python ;)15:33
studiobot<teward001> Eickmeyer: yikes, there was build issues earlier15:33
Eickmeyer@teward001: cjwatson said they were doing a test rebuild.15:34
studiobot<teward001> that would explain it15:35
tewardEickmeyer: if you need me to fast-build I can compile for amd64 for you to test sooner16:34
tewardbut you'll of course have to download the .deb and install16:34
Eickmeyerteward: 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:35
tewardi'll sbuild it on eoan amd64 local then upload somewhere standby16:36
EickmeyerI stand corrected, it just published.16:37
* Eickmeyer is installing the built deb now16:37
tewardheh16:38
tewardwell that was a fast build now xD16:38
tewardEickmeyer: https://people.ubuntu.com/~teward/ubuntustudio-menu-add_0.1_all.deb if the PPA is slow :P16:39
tewardyay for sbuild :)16:39
Eickmeyerteward: Already have it. Everything works from my tests, but OvenWerks might have some more in-depth testing.16:40
EickmeyerOvenWerks: New build in autobuilds, you can probably just apt upgrade.16:40
tewardand if that doesn't work for you I have the built binary in that link above :)16:41
tewardEickmeyer: I almost totally forgot about 'people.u.c' though as an SFTP upload point xD16:41
* Eickmeyer hasn't used people.u.c, and probably could.16:41
EickmeyerOvenWerks: Let us (me & teward) know how testing ubuntustudio-menu-add goes.17:44
studiobot* teward001 was poked17:44
studiobot* teward001 goes back to poking redis-server instead17:44
Eickmeyerhehe17:44
* teward drops a crate of onions on Eickmeyer17:45
tewardoops :)17:45
EickmeyerMMmmm... walla walla sweets...17:45
* Eickmeyer starts grilling the onions for burgers17:47
* sarnold spills some kosher salt and slices off a big piece of onion17:47
Eickmeyer^this guy onions.17:48
tewardi'm sorry you assume that sarnold is NOT ALREADY an onion :p17:49
teward*shot*17:49
OvenWerksEickmeyer: 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
EickmeyerOvenWerks: No worries. teward^17:50
* Eickmeyer wanders off to play a game with his son17:51
tewardack17:54
tewardOvenWerks: i miiight have a way to make the 'subprocess.run' with an array work18:17
tewardbut we need to drop shell=True possibly18:17
teward(to sate sarnold's concerns)18:17
tewardOvenWerks: Eickmeyer: specifically, http://people.ubuntu.com/~teward/array-based_subprocess_execution_of_exo-desktop-item-edit.patch18:24
tewardwhich i THINK works proper18:24
tewardbut again, needs the tests :)18:24
tewardno rush18:24
tewardEickmeyer: OvenWerks: http://people.ubuntu.com/~teward/ubuntustudio-menu-add_0.1_all_v2.deb <-- with that last patch18:27
tewardin case you want to test :)18:27
teward(since the builders are slow)18:28
OvenWerksteward: I think the terminal was there because it uses the binary name without the path.18:28
tewardpossibly, but I don't think hwe have a choice so longa s the binary is in PATH18:28
OvenWerksagain one of those things I tried more than one way18:28
tewardOvenWerks: we could also hardcode the full path18:28
tewardwhich is probably the even safer approach18:29
tewardi don't know the full path though for that binary18:29
OvenWerksI don't suppose it is likely to move from one cycle to the next ;)18:29
tewardyeah, in which case hardcode it18:29
tewardi know that hardcoding things like that *tends* to be a bad thing18:29
tewardbut we can always patch code later if things cahnge :P18:29
OvenWerks$ which exo-desktop-item-edit 18:29
OvenWerks/usr/bin/exo-desktop-item-edit18:30
tewardchange*18:30
tewardpatch updated i'll update that .deb :p18:31
OvenWerksusing the full path is always considered more secure18:31
studiobot<teward001> yes, it is18:31
OvenWerkspk won't allow anything else18:32
studiobot<teward001> http://people.ubuntu.com/~teward/ubuntustudio-menu-add_0.1_all_v3.deb is the 3rd deb version then including the fullpath this time18:33
studiobot<teward001> again, in case you all want to supersede the speed of the builders ;)18:33
OvenWerksteward: I won't be able to test for a few hours (prolly 1430 local (-0700))18:34
tewardthat's fine18:34
tewardas i said no rush18:34
tewardbut the builders are stressed right now18:34
tewardso :P18:34
OvenWerksSpending time with my Yf before she goes to work.18:35
tewardno problem :)18:35

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!