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

Support debian packaging #49

Merged
merged 8 commits into from
Oct 23, 2018
Merged

Support debian packaging #49

merged 8 commits into from
Oct 23, 2018

Conversation

joshuathayer
Copy link
Contributor

This removes PyQt from management via pipenv, and adds instructions to install system-managed PyQt packages instead. @kushaldas can explain further, but this has to do with our desire to use only packages we can identify by checksum.

This also adds certifi, which I needed to add in order to get the packaged software to run after installation.

The other additions to Pipfile.lock are transitive from securedrop-sdk

This PR also adds requirements-build.txt, per @kushaldas 's recommendation:

Remember to commit the requirements-build.txt file to the git repo. This will help others to build using the same source tarballs.

@@ -9,10 +9,10 @@ python_version = "3.5"
[packages]
SQLALchemy = "*"
alembic = "*"
securedrop-sdk = {git = "https://github.com/freedomofpress/securedrop-sdk.git"}
securedrop-sdk = "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

ah right, so the workflow from now when we make a change on the sdk side that we need in the journalist GUI will be us making a new release of the securedrop-sdk and updating on PyPI, i'll do this since we need a release after this change was made: freedomofpress/securedrop-sdk#21

Copy link
Contributor

Choose a reason for hiding this comment

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

@redshiftzero Yup, I did a 0.0.1 release so that we can test this packaging story. I think we should do a 0.0.2 release as that will allow us to experiment more as far as the version number goes.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep i agree

@@ -51,16 +72,6 @@
"index": "pypi",
"version": "==2.3.2"
},
"pyqt5": {
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: instead of removing pyqt5 entirely we should probably move pyqt5 to the development requirements of the Pipfile, since we still want to make sure it's installed in dev/ci/test but not prod/build

Copy link
Contributor

Choose a reason for hiding this comment

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

This still can create false positive as dev/ci will be different that production. We should use the same system provided package everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using a pipfile dev dependency enables e.g. macOS developers. If the only way to run the dev env is to use a Debian-based OS, that's going to be a problem. Probably worth accepting a small bit of divergence in order to accommodate dev workflows here.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i mean, i'm open to other suggestions here but the only other real option (other than use debian, ha) is to instruct developers on macOS to install qt5 separately in the dev env

Copy link

Choose a reason for hiding this comment

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

dumb question here -- can we use docker containers and VNC ? if we use system level PyQt versions arent we going to have mismatch between different developers? or is pyqt so rock stable that this shouldn't be a problem? (i swear im not trolling here!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point @msheiny, with system packages, it's not just "Debian-based OS" that'd be the requirement, but specifically Debian Stretch. Unless you're running Qubes, that's a lot to ask from contributors looking to write a bit of Python code.

The container/VNC strategy is interesting, but given the outstanding deadlines we're working with—particularly the upcoming audit—I suggest we preserve the pyqt dependency in the pipfile via dev dependencies as @redshiftzero suggests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed a commit which adds PyQt5 to the dev dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on all this...WRT PyQT stability issues... I'd say it's "mostly" rock solid, except when it isn't. You can draw your own conclusions. ;-)

@heartsucker
Copy link
Contributor

but this has to do with our desire to use only packages we can identify by checksum.

Uh ok so maybe I'm missing something, but don't we have the sha256 hashes of the package in the lockfile? And also aren't we using pip wheel to include them in the debian package? It seems overly complex to have some python requirements be in the lock file and some come from system and could also lead to divergences and incompatibilities.

@kushaldas
Copy link
Contributor

Uh ok so maybe I'm missing something, but don't we have the sha256 hashes of the package in the lockfile? And also aren't we using pip wheel to include them in the debian package? It seems overly complex to have some python requirements be in the lock file and some come from system and could also lead to divergences and incompatibilities.

PyQt directly releases binary wheels to PyPI. We don't know how it is built. Building PyQt dependencies from source (we have to get source from the official website as they are missing in PyPI) is a lot of extra work to maintain. It is much easier to use system provided PyQt5.

@heartsucker
Copy link
Contributor

Right got it. Totally makes sense.

@ntoll
Copy link
Contributor

ntoll commented Oct 22, 2018

I see the tests have failed. :-/

@kushaldas
Copy link
Contributor

I see the tests have failed. :-/

To make that test pass, we will have to do a new release of securedrop-sdk, which is freedomofpress/securedrop-sdk#24. But, this is blocked on freedomofpress/securedrop-sdk#26

@kushaldas
Copy link
Contributor

Also we should down the setup.py to 0.0.1 so that we count these releases as really alpha level.

chardet==3.0.4 --hash=sha256:84ab92ed1c4d4f16916e05906b6b75a6c0fb5db821cc65e70cbd64a3e2a5eaae --hash=sha256:fc323ffcaeaed0e0a02bf4d117757b98aed530d9ed4531e3e15460124c106691
alembic==1.0.1 --hash=sha256:0fe570f23dc48fb1bbda6f6a396f1c0c28d7045c0ad14018c104a511e6c1fe8a
sqlalchemy==1.2.12 --hash=sha256:c5951d9ef1d5404ed04bae5a16b60a0779087378928f997a294d1229c6ca4d3e
securedrop-sdk==0.0.1 --hash=sha256:82373118c49a141881332575c9ac1618973390a7d5c32ed29b3d64cb1f0d91e8
Copy link
Contributor

Choose a reason for hiding this comment

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

note for other maintainers: these are the build requirements from the first attempt building the securedrop-client package. For the next package build we will need to refresh these build requirements (to pull in securedrop-sdk==0.0.2)

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

CI now passing here as we have a new version of securedrop-sdk, merging this in

@redshiftzero redshiftzero merged commit 76e273e into master Oct 23, 2018
@redshiftzero redshiftzero deleted the packaging-fun branch October 23, 2018 23:17
legoktm pushed a commit that referenced this pull request Dec 11, 2023
Adds wheel hashes for both Stretch and Buster
legoktm pushed a commit that referenced this pull request Dec 15, 2023
Adds optional client side uuid selection for replies
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.

7 participants