Tails

#1 Updated by intrigeri 2019-02-21 08:03:15

• Assignee set to alant

@alant, can you please take a look?

BTW, I suspect IBus will be broken there as well.

#2 Updated by anonym 2019-02-21 09:27:21

• Assignee changed from alant to anonym
• % Done changed from 0 to 50

intrigeri wrote:
> @alant, can you please take a look?

I actually already started looking into this yesterday (I wrote a comment abou it but only pressed “Preview”, not “Submit”…) so I hope it is ok that I take it. I want to know the fundamentals around AT-SPI a bit better for future Dogtail work.

> BTW, I suspect IBus will be broken there as well.

Good call!

#3 Updated by anonym 2019-02-21 11:37:45

• Status changed from Confirmed to In Progress

Applied in changeset commit:tails|366124493a9c55a1db62caccfcd28d59fccbc1eb.

#4 Updated by anonym 2019-02-21 11:41:39

• Assignee deleted (anonym)
• QA Check set to Ready for QA
• Feature Branch set to tails: bugfix/16475-asp-notifications-gnome-env submodules/pythonlib: bugfix/16475-asp-notifications-gnome-env

Fixed! Turns out IBus still worked, and only XDG_RUNTIME_DIR was missing in the environment for AT-SPI.

To verify the fix:

1. Boot an image builtfFrom this branch
2. Set an admin password (for apt)
3. sudo apt update
4. sudo apt install cowsay (or whatever)
5. Wait for the notification to appear; click the “Documentation” button
6. Wait for Tor Browser to start
7. Verify that Tor Browser’s AT-SPI works with:
   1. Make sure this doesn’t print an exception: python2 -c "from dogtail import tree; tree.root.application('Firefox', retry=False)"
   2. Enable the screen reader and made sure it works in Tor Browser

#5 Updated by intrigeri 2019-02-21 16:16:29

• Assignee set to intrigeri

#6 Updated by intrigeri 2019-02-22 07:44:41

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Info Needed

@anonym, here’s a code review:

• Why is most of the code from gnome_env_dict duplicated at the top of gnome.py?
• What is the export_gnome_env function for?
• I wonder if we really need to export these environment variables here (it seems we don’t); if we don’t, I’m slightly worried about side effects.
• I doubt we really need to use the low-level subprocess.Popen here: “The recommended approach to invoking subprocesses is to use the run() function for all use cases it can handle” (https://docs.python.org/3/library/subprocess.html).

#7 Updated by anonym 2019-02-22 09:08:33

• Assignee changed from anonym to intrigeri
• % Done changed from 50 to 60
• QA Check changed from Info Needed to Ready for QA

intrigeri wrote:
> anonym, here's a code review: > > * Why is most of the code from gnome_env_dict@ duplicated at the top of gnome.py?

I was too lazy to refactor (actually I’m surprised!). Fixed!

> * What is the export_gnome_env function for?

It is the exact analog of the shell wrapper’s export_gnome_env. It was my first attempt, but after having similar concerns as you in your next point I chose the ... 'env', *gnome_env_vars ...() approach instead. I chose to leave export_gnome_env around since I thought it could help in our shell → python porting effort nt I now realize that the other approach I came up with probably always will be superior, so let’s drop everything else.

> * I wonder if we really need to export these environment variables here (it seems we don’t); if we don’t, I’m slightly worried about side effects.

We don’t; we just set the relevant variables (via gnome_env_vars()) in the subprocess’ env.

> * I doubt we really need to use the low-level subprocess.Popen here: “The recommended approach to invoking subprocesses is to use the run() function for all use cases it can handle” (https://docs.python.org/3/library/subprocess.html).

Thanks for the tip!

I think that fixes all your concenrs so far. I’m much happier with the state of gnome.sh now so thanks again!

#8 Updated by intrigeri 2019-02-22 09:37:04

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Dev Needed

It seems that you did not push your submodules changes. I guess that’s one drawback of allowing usage of local-only submodule refs: nothing forces anyone to push their commits until the reviewer notices the problem. This would not be a problem if we followed https://tails.boum.org/contribute/merge_policy/#submit more strictly: you would have noticed that this makes the branch FTBFS on Jenkins.

#9 Updated by anonym 2019-02-22 10:23:07

• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> It seems that you did not push your submodules changes.

Sorry, pushed!

> I guess that’s one drawback of allowing usage of local-only submodule refs: nothing forces anyone to push their commits until the reviewer notices the problem.

True.

> This would not be a problem if we followed https://tails.boum.org/contribute/merge_policy/#submit more strictly: you would have noticed that this makes the branch FTBFS on Jenkins.

Lately I’ve definitely been too eager about throwing my tickets on other people for QA. Sorry about this! Like I used to do in the past, I’ll start to assign tickets to myself as RFQA with a note to “Let’s see what jenkins thinks” and then look back the next day.

So, let’s see what jenkins thinks! :)

#11 Updated by anonym 2019-02-22 15:14:38

• Assignee changed from anonym to intrigeri

Jenkins build and tested the image successfully: https://jenkins.tails.boum.org/view/Tails_ISO_builds/job/test_Tails_ISO_bugfix-16475-asp-notifications-gnome-env/3/

#12 Updated by intrigeri 2019-02-23 06:57:37

• Assignee changed from intrigeri to anonym

Hi @anonym,

code review now passes! A couple comments below though.

>> * I wonder if we really need to export these environment variables here (it seems we don’t); if we don’t, I’m slightly worried about side effects.

> We don’t; we just set the relevant variables (via gnome_env_vars()) in the subprocess’ env.

Right, sorry I was confused: we’re calling export_gnome_env(), which does export the variables in a way that’s visible from a shell script that uses it and its future child processes; but in the case at hand, since we’re running this shell code in a child process, export has no side effect on the parent Python program.

>> * I doubt we really need to use the low-level subprocess.Popen here: “The recommended approach to invoking subprocesses is to use the run() function for all use cases it can handle” (https://docs.python.org/3/library/subprocess.html).

> Thanks for the tip!

Great.

In passing, I think a more idiomatic way to use this would be to:

• use subprocess.run’s built-in check=True functionality and drop the ad-hoc assert proc.returncode == 0
• use capture_output=True instead of the lower level stdout = subprocess.PIPE (by the way, I think one does not insert spaces around the equal sign in Python there)

But I’m not going to block on this.

The fix on this branch is targeted and minimal enough for me to feel comfortable merging into stable once I’ll have built and successfully tested.

#13 Updated by intrigeri 2019-02-23 07:41:27

• Assignee changed from anonym to intrigeri

anonym wrote:
> To verify the fix:
>
> # Boot an image builtfFrom this branch

FTR this must be an ISO image, otherwise there’s no “Documentation” link in the notification. (These days, when I read “image”, I assume “USB image”.)

#14 Updated by intrigeri 2019-02-23 07:49:42

• Assignee changed from intrigeri to anonym
• % Done changed from 60 to 70
• QA Check changed from Ready for QA to Info Needed

It works!

anonym, I took a step back and spotted a potential design issue: here we're patching _notify, which is merely a wrapper around /usr/local/lib/tails-additional-software-notify. So tails-additional-software-notify@ will still be broken if it’s ever used from elsewhere, without going through the code you’re adding to _notify. It would feel better design to me if we fixed tails-additional-software-notify itself. What do you think?

#15 Updated by anonym 2019-02-25 09:51:37

• Assignee changed from anonym to intrigeri
• QA Check changed from Info Needed to Ready for QA

intrigeri wrote:
> >> * I wonder if we really need to export these environment variables here (it seems we don’t); if we don’t, I’m slightly worried about side effects.
>
> > We don’t; we just set the relevant variables (via gnome_env_vars()) in the subprocess’ env.
>
> Right, sorry I was confused: we’re calling export_gnome_env(), which does export the variables in a way that’s visible from a shell script that uses it and its future child processes; but in the case at hand, since we’re running this shell code in a child process, export has no side effect on the parent Python program.

Yes, that was how it worked when export_gnome_env() existed but now there is only gnome_env_vars() which doesn’t touch the environment, but just return a list of VAR=VALUE:s for the four “GNOME” variables, that we pass to env that is wrapping the child process.

> >> * I doubt we really need to use the low-level subprocess.Popen here: “The recommended approach to invoking subprocesses is to use the run() function for all use cases it can handle” (https://docs.python.org/3/library/subprocess.html).
>
> > Thanks for the tip!
>
> Great.
>
> In passing, I think a more idiomatic way to use this would be to:
>
> * use subprocess.run’s built-in check=True functionality and drop the ad-hoc assert proc.returncode == 0
> * use capture_output=True instead of the lower level stdout = subprocess.PIPE (by the way, I think one does not insert spaces around the equal sign in Python there)

Nice! capture_output is too new for Tails’ python3 (3.5.3, we need 3.7), but I found subprocess.check_output() which does exactly what we want.

> FTR this must be an ISO image, otherwise there’s no “Documentation” link in the notification. (These days, when I read “image”, I assume “USB image”.)

Aha! I will update my terminology! :)

> anonym, I took a step back and spotted a potential design issue: here we’re patching _notify, which is merely a wrapper around /usr/local/lib/tails-additional-software-notify. So tails-additional-software-notify will still be broken if it’s ever used from elsewhere, without going through the code you’re adding to _notify. It would feel better design to me if we fixed tails-additional-software-notify itself. What do you think?

Sure! I was definitely mirroring the design of tails-notify-user too much! :P

#16 Updated by intrigeri 2019-02-25 15:12:03

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Info Needed

> Nice! capture_output is too new for Tails’ python3 (3.5.3, we need 3.7), but I found subprocess.check_output() which does exactly what we want.
> […]
>> anonym, I took a step back and spotted a potential design issue: here we’re patching _notify, which is merely a wrapper around /usr/local/lib/tails-additional-software-notify. So tails-additional-software-notify will still be broken if it’s ever used from elsewhere, without going through the code you’re adding to _notify. It would feel better design to me if we fixed tails-additional-software-notify itself. What do you think?

> Sure! I was definitely mirroring the design of tails-notify-user too much! :P

Did you forget to push, or?

#17 Updated by anonym 2019-02-28 08:05:17

• Assignee changed from anonym to intrigeri
• % Done changed from 70 to 60
• QA Check changed from Info Needed to Ready for QA

Yeah, this time I remembered to push the submodule but not the main repo.

#18 Updated by intrigeri 2019-03-04 13:33:46

• Status changed from In Progress to Fix committed
• % Done changed from 60 to 100

Applied in changeset commit:tails|b66cb28f7958dcf28ea664a76f05b38d59dd603e.

#19 Updated by intrigeri 2019-03-04 13:35:10

• Assignee deleted (intrigeri)
• QA Check changed from Ready for QA to Pass

#20 Updated by CyrilBrulebois 2019-03-20 14:25:58

• Status changed from Fix committed to Resolved