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

create prod-like dev env #1301

Merged
merged 3 commits into from
Sep 27, 2021
Merged

create prod-like dev env #1301

merged 3 commits into from
Sep 27, 2021

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Sep 17, 2021

Description

Fixes issues where the gui looks different between prod and dev by supporting a dev env that uses system site-packages that you have to install ahead of time (see https://github.com/freedomofpress/securedrop-debian-packaging/blob/main/securedrop-client/debian/control).

Test Plan

  1. install these packages on your debian machine (this can be a vm in qubes or native install): https://github.com/freedomofpress/securedrop-debian-packaging/blob/main/securedrop-client/debian/control
  2. make venv-debian && source .venv-debian/bin/activate
  3. run the client (./run.sh) against a dev server

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 requested a review from a team as a code owner September 17, 2021 21:31
@sssoleileraaa sssoleileraaa force-pushed the create-prod-like-dev-env branch from 06b7c06 to 107375a Compare September 17, 2021 21:32
@sssoleileraaa sssoleileraaa force-pushed the create-prod-like-dev-env branch from 107375a to 26188ee Compare September 23, 2021 00:32
@sssoleileraaa
Copy link
Contributor Author

just rebased onto main and added babel to the new dev requirements for debian and then ran make requirements here: 26188ee

@sssoleileraaa sssoleileraaa force-pushed the create-prod-like-dev-env branch from 26188ee to a7b303d Compare September 23, 2021 01:07
@sssoleileraaa
Copy link
Contributor Author

I'm seeing an issue with make test freezing when using the new virtual environment. It seems to be a test runner issue because tests pass when you run each test file individually, e.g. TESTS=tests/gui/test_widgets.py make test, but not when you run make test

@conorsch
Copy link
Contributor

Started review, and appended a commit tweaking the venv creation. Using that commit, I could not reproduce the make test hang that you reported, @creviera. Mind trying again with the rebased version here?

@conorsch
Copy link
Contributor

@creviera These changes work well enough for me. As I mentioned in #1286, I need a tor connection (for the latency) to reproduce that particular issue, but otherwise the changes here look great to me. My major question at this point is: what's the value of supporting the old-school, non-system-packages "venv" at all? In other words, shouldn't this new approach to reusing the Debian system packages be the default? I'd expect that the non-debian venv would be useful to folks developing on e.g. macos, but I haven't actuall tested that. Thoughts?

Allie Crevier and others added 3 commits September 27, 2021 10:18
Signed-off-by: Allie Crevier <[email protected]>
The reminder to use system qt packages is probably better served by a
script, but not bothering with that now.
@conorsch conorsch force-pushed the create-prod-like-dev-env branch from 6b1c713 to 15cf0fe Compare September 27, 2021 17:19
@conorsch
Copy link
Contributor

I pared back my commit to add only comments, since @creviera rightly pointed out that the manually source actions weren't necessary. This is working well on my machine, which is a Qubes AppVM based on Debian 10 buster. I've not tested on a bare metal install of Debian 10, since I just upgraded my bare metal installation to 11 as part of freedomofpress/securedrop-dev-docs#9. @creviera did note that on her bare metal install, the make test hangs—but neither she nor I can reproduce in a Debian AppVM. This is definitely enough of an improvement that I'm comfortable merging as-is, without any further changes to e.g. CI. Will approve and leave merge to you, @creviera, assuming you agree. =)

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Works well enough for me! Will be performing a lot of repro in the future, will follow up with any required changes as they present themselves.

@sssoleileraaa
Copy link
Contributor Author

@creviera These changes work well enough for me. As I mentioned in #1286, I need a tor connection (for the latency) to reproduce that particular issue, but otherwise the changes here look great to me. My major question at this point is: what's the value of supporting the old-school, non-system-packages "venv" at all?

conor and i discussed in person and will leave both .venv and .venv-debian since using system site packages can cause some issues with pytest freezing up. until that's for sure fixed, it makes sense to support both.

@sssoleileraaa
Copy link
Contributor Author

thanks, conor, for the review and troubleshooting (and for leaving the fun button pushing for me at the end)

@sssoleileraaa sssoleileraaa merged commit ee4f55f into main Sep 27, 2021
@sssoleileraaa sssoleileraaa deleted the create-prod-like-dev-env branch September 27, 2021 23:27
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.

2 participants