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

Update client dev environment for bullseye #1496

Merged
merged 1 commit into from
Jul 6, 2022

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented May 25, 2022

Description

Closes #1513

Since the next release of the client will be targeted for Bullseye, we can update our dev environment versions for pyqt5 and sip to match what gets installed in the prod env: https://github.com/freedomofpress/securedrop-debian-packaging/blob/5dad39315f76443f20121a1c01acfb3be584453b/securedrop-client/debian/control#L12

While I was at it, I updated all dev-only dependencies, keeping that commit separate.

Test Plan

In a Bullseye VM:

  • Confirm rm -r .venv && make venv && source .venv/bin/activate creates a dev env without error
  • Start the client and see that it is working as expected with no regressions
  • Hop over to sd-app where pyqt is installed, temporarily give it network access (this is on a dev machine), clone the client repo and checkout this PR branch
  • Confirm make venv-sdw && source .venv/bin/activate creates a dev env using system site-packages without error
  • Start the client and see that it is working as expected with no regressions

In a Buster VM:

  • Confirm rm -r .venv && make venv-buster && source .venv/bin/activate creates a dev env without error
  • Start the client and see that it is working as expected with no regressions
  • Hop over to sd-app where pyqt is installed, temporarily give it network access (this is on a dev machine), clone the client repo and checkout this PR branch
  • Confirm make venv-sdw && source .venv/bin/activate creates a dev env using system site-packages without error
  • Start the client and see that it is working as expected with no regressions

In a Bullseye VM:

  • Confirm make requirements results in no local changes
  • Confirm make update-dev-dependencies results in only updates to the dev-*requirements.txt files (you should see lots of newer versions of dev dependencies)

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:

  • I have updated the AppArmor profile
  • No update to the AppArmor profile is required for these changes
  • I don't know and would appreciate guidance

If these changes modify the database schema, you should include a database migration. Please check as applicable:

  • I have written a migration and upgraded a test database based on main and confirmed that the migration applies cleanly
  • I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
  • I need help writing a database migration
  • No database schema changes are needed

@sssoleileraaa sssoleileraaa force-pushed the update-dev-deps-to-match-bullseye-env branch 3 times, most recently from 6924006 to c0718e6 Compare May 26, 2022 02:36
@gonzalo-bulnes gonzalo-bulnes self-requested a review May 26, 2022 02:43
@sssoleileraaa sssoleileraaa force-pushed the update-dev-deps-to-match-bullseye-env branch 2 times, most recently from db5e98d to e418d5d Compare May 26, 2022 02:45
@sssoleileraaa sssoleileraaa marked this pull request as ready for review May 26, 2022 02:46
@sssoleileraaa sssoleileraaa requested a review from a team as a code owner May 26, 2022 02:46
@gonzalo-bulnes
Copy link
Contributor

Mmmm, make test is green locally, but make check fails with a core dump (:open_mouth:) which does look like the one in CI (error in CI). Because it happened consistently in my local environment and also in CI, I suppose that's a legit failure?

I'm not sure what could be causing it, and haven't looked much into it. Happy to troubleshoot together if you want! 🕵️

@sssoleileraaa sssoleileraaa force-pushed the update-dev-deps-to-match-bullseye-env branch 2 times, most recently from 74592d3 to 04cc962 Compare May 26, 2022 16:59
@sssoleileraaa
Copy link
Contributor Author

@gonzalo-bulnes, it looks like this works locally because we are both testing with python3.9. I tried switching from buster to bullseye images for CI, since there will be no more client releases targeted for buster, but now I'm realizing that there's still a period where we might need to make a hot fix for buster builds. So what I think we need here is two additional CI jobs for bullseye, and separate dependency files until we completely remove support for buster.

@sssoleileraaa sssoleileraaa force-pushed the update-dev-deps-to-match-bullseye-env branch 2 times, most recently from 6100819 to bfe24cb Compare May 26, 2022 21:47
@sssoleileraaa
Copy link
Contributor Author

Seeing the following error for the new bullseye jobs, so I'll have to figure out why that's not working later (but at least the buster jobs are back to normal on this branch):

E: Could not open lock file /var/lib/apt/lists/lock - open (13: Permission denied)

@sssoleileraaa
Copy link
Contributor Author

Just ssh-ing into the CI box and running each line one by one and I can see we need to use sudo to fix the error above, but onto the next error...

@sssoleileraaa sssoleileraaa force-pushed the update-dev-deps-to-match-bullseye-env branch 2 times, most recently from fd7e808 to 0c33802 Compare May 26, 2022 22:28
@sssoleileraaa
Copy link
Contributor Author

Okay we are looking much better now! make test and make test-integration is passing, and looks like I'm down to an issue with the functional test runner...

@sssoleileraaa
Copy link
Contributor Author

sssoleileraaa commented May 26, 2022

Looks like we can only run one functional test at a time in bullseye with our current setup. I checked that each functional test can run by itself, and only that we only see this issue when we use our functional test runner script: https://github.com/freedomofpress/securedrop-client/blob/main/test-functional.sh

@sssoleileraaa
Copy link
Contributor Author

sssoleileraaa commented Jun 6, 2022

@gonzalo-bulnes - since this is towards bullseye/4.1 support, and it looks like there might be a bit of work to get the functional tests working for bullseye, I was wondering if you could spend some of your client dev time helping me get this over the finish line? I'm not sure how much time I'll have to work on this this week, but would appreciate your help so that this doesn't end up blocking the next release!

@gonzalo-bulnes
Copy link
Contributor

Yes, happy to jump in @creviera, this looks to me like more pressing for the next release than the "Export All Files" feature, isn't it?

I think it would make sense to focus on unblocking dev on Bullseye by focusing on this issue fist, then we could apply some pair focus on "Export All Files". That would have the advantage of delivering the Bullseye work first, instead of more slowly working on both topics in parallel.

@gonzalo-bulnes gonzalo-bulnes self-assigned this Jun 6, 2022
@sssoleileraaa sssoleileraaa changed the title Update dev deps to match bullseye env Update client dev environment and test suite for bullseye Jun 7, 2022
@gonzalo-bulnes gonzalo-bulnes force-pushed the update-dev-deps-to-match-bullseye-env branch from 80d5a54 to 0c33802 Compare June 7, 2022 02:20
@gonzalo-bulnes
Copy link
Contributor

gonzalo-bulnes commented Jun 7, 2022

(--force-with-lease push) I only removed a commit I had added.

@gonzalo-bulnes
Copy link
Contributor

gonzalo-bulnes commented Jun 7, 2022

@creviera @eloquence I left a detailed comment in #1512 with the current status. As far as I can tell from the progress made in #1512 (fixing the functional test suite), the hypothesis of segfaults due to thread management missing bits (#1510) seems to hold water. 🍉

With that in mind, I've outlined a way forward after #1512 in a comment, and left some work in progress linked there. 🙂
@creviera Let's have a chat tomorrow!

@gonzalo-bulnes
Copy link
Contributor

Test suite fixed 👯 (Buster and Bullseye) I'll clean up the branch and package it as a PR tomorrow 🙂

@sssoleileraaa
Copy link
Contributor Author

@gonzalo-bulnes - I rebased after merging #1515 and see test-bullseye is still failing. You mentioned that there would be 2 separate PRs that address the test-bullseye failure- which PR is up next?

@sssoleileraaa
Copy link
Contributor Author

Ah, looks like this is up next: #1512 (comment)

@gonzalo-bulnes
Copy link
Contributor

gonzalo-bulnes commented Jun 21, 2022

@creviera You're correct, it's after #1512 that rebasing will fix this build 🙂 (original comment and proof of concept).

@sssoleileraaa sssoleileraaa force-pushed the update-dev-deps-to-match-bullseye-env branch from 08160ac to a506fa5 Compare June 22, 2022 01:19
@sssoleileraaa
Copy link
Contributor Author

@gonzalo-bulnes - this now passes CI, but I'd like to do a bit more cleanup around our dev env and how we manage requirements as I fix up errors around make venv-sdw and make requirements.

@sssoleileraaa
Copy link
Contributor Author

@gonzalo-bulnes - this is on my radar, just haven't had time to finish up yet

@sssoleileraaa
Copy link
Contributor Author

TODO: If I can't get this over the finish line tomorrow, then update this with a checklist for someone else

@gonzalo-bulnes
Copy link
Contributor

gonzalo-bulnes commented Jun 29, 2022

Yes, all good @creviera. Please feel free to hand-ball it to me any time. 🏉 🙌

@sssoleileraaa
Copy link
Contributor Author

@gonzalo-bulnes - At this point, i updated the test plan to include both sd-app and non sd-app environments for both buster and bullseye. CI is still spinning so if there are any failures there or that you find while running through the test plan, feel free to either append a fix or just report it here. For the test plan, I ran out of time so I only confirmed that: 1) I could create the dev environments in all four scenarios, 2) I could start the client in all four scenarios, and 3) I am able to update requirements files in a bullseye dev env (let's not worry about doing this for buster because it'll mean more duplicate code)

@gonzalo-bulnes
Copy link
Contributor

gonzalo-bulnes commented Jul 5, 2022

Test Plan

🍏 GOOD
🍊 CAUTION
🔴 BAD

In a Bullseye VM:

  • Confirm rm -r .venv && make venv && source .venv/bin/activate creates a dev env without error
  • make check succeeds 🍏 covered by CI but I like to see it locally nonetheless)
  • Start the client and see that it is working as expected with no regressions 👈 I'll be able to test this one properly tomorrow! 🍊
  • Hop over to sd-app where pyqt is installed, temporarily give it network access (this is on a dev machine), clone the client repo and checkout this PR branch
  • Confirm make venv-sdw && source .venv/bin/activate creates a dev env using system site-packages without error
  • Start the client and see that it is working as expected with no regressions

In a Buster VM:

  • 🍌 python3 --version is Python 3.7.3
  • Confirm rm -r .venv && make venv-buster && source .venv/bin/activate creates a dev env without error 🍏
    Note: That is make venv-buster! Using make venv yields unexpected results. 😉
  • make check succeeds 🍏 (covered by CI but I like to see it locally nonetheless)
  • Start the client and see that it is working as expected with no regressions
  • Hop over to sd-app where pyqt is installed, temporarily give it network access (this is on a dev machine), clone the client repo and checkout this PR branch
  • Confirm make venv-sdw && source .venv/bin/activate creates a dev env using system site-packages without error
  • Start the client and see that it is working as expected with no regressions

In a Bullseye VM:

  • Confirm make requirements results in no local changes 🍊
    I noticed something I can't quite explain (both in Bullseye and Buster by the way):
    • On first run, make requirements would fail. The error (ImportError: cannot import name 'BAR_TYPES' from 'pip._internal.cli.progress_bars') lead me to Error importing BAR_TYPES with new pip 22.1 release jazzband/pip-tools#1617 and to updgrading pip-tools form 6.5.1 to the latest 6.8.0.
    • With pip-tools==6.8.0, make requirements succeeds.
    • When removing the virtual environment and starting again, make requirements succeeds even when using pip-tools==6.5.1 😮 💥
    • And by the way, it adds no changes on Bullseye, as expected 🍏
  • Confirm make update-dev-dependencies results in only updates to the dev-*requirements.txt files (you should see lots of newer versions of dev dependencies)
    🍊 Note that with the exception of one mypy error that's trivial to fix, make check still succeeds after applying all the dev dependency upgrades. May we want to include those in this PR?
    diff --git a/securedrop_client/queue.py b/securedrop_client/queue.py
    index b37aa60..4b458fe 100644
    --- a/securedrop_client/queue.py
    +++ b/securedrop_client/queue.py
    @@ -92,7 +92,8 @@ class RunnableQueue(QObject):
             stored on self.current_job. We check that the job to be added is not among them.
             """
             in_progress_jobs = [in_progress_job for priority, in_progress_job in self.queue.queue]
    -        in_progress_jobs.append(self.current_job)
    +        if self.current_job is not None:
    +            in_progress_jobs.append(self.current_job)
             if job in in_progress_jobs:
                 logger.debug("Duplicate job {}, skipping".format(job))
                 return True

@gonzalo-bulnes
Copy link
Contributor

Hi @creviera! Te test plan is checking out so far. Tomorrow I'll be able to confirm that the client runs as expected in a Bullseye sd-dev. (It's just taken forever to actually update the VM.)

I don't have sd-apps on development machines, so let's have a chat tomorrow 🙂

@sssoleileraaa sssoleileraaa force-pushed the update-dev-deps-to-match-bullseye-env branch 6 times, most recently from baaa6fb to d7cd4bb Compare July 6, 2022 01:48
@sssoleileraaa sssoleileraaa force-pushed the update-dev-deps-to-match-bullseye-env branch from 5798cae to 81e11a9 Compare July 6, 2022 01:59
Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@creviera And I have tested this over the last two days. It looks good both on Buster and Bullseye.

@gonzalo-bulnes gonzalo-bulnes merged commit 2691c35 into main Jul 6, 2022
@gonzalo-bulnes gonzalo-bulnes deleted the update-dev-deps-to-match-bullseye-env branch July 6, 2022 02:41
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

Successfully merging this pull request may close these issues.

Add Python 3.9 support
2 participants