This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these items not go in
build-packages
instead ofpython-packages
? They shouldn't be required to run Synapse.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, yes, however the
build-packages
setting requires that apt/debian packages are used. So, there are a few approaches possible here, and I'm happy to use the one you are all comfortable with -python-packages
setting. Upside is it's clean and self-documenting, downside is the dependencies are also installed in the snap, not just at build time.build-packages
setting. Upside would be that the packages are only installed during build and would not be staged for inclusion in the snap. Downside, requires the use for Debian packages, so could not pull the latest or potentially the required versions from Pip.override-build
command which included a step to manuallypip install
the required build tools. Upsides: should not be staged (in theory, would require testing) and can use the pip versions of the packages. Would more closely mirror the README, also. Downsides: Messy and not self-documenting in the snapcraft file, does not follow regular snapcraft metadata and may be more difficult to maintain in the future.Let me know how you'd like me to tackle it, and I'll make the required changes. Currently, the snap build is failing due to a missing
setuptools_scm
module, which did not occur previously, so figured it best to explicitly mirror the build documentation by making sure these packages are installed from pypi during build.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the detailed overview. I agree that I'm a little hesitant to rely on distro packages here when snap is designed precisely to not be that.
I think I'm ok with including them in
python-packages
, acknowledging that will bloat the package a bit, but it's a tradeoff with not having things break down the line with a complicated install script or dependencies shifting out of our control.