[DPE-3062] Removing binary deps #179

dragomirp · 2024-01-16T21:18:45Z

Remove charmcraft binary python packages and instead bundle necessary SO libraries to build from source.

dragomirp · 2024-01-17T15:45:48Z

charmcraft.yaml

+      - libpq-dev
+  libpq:
+    build-packages:
+      - libpq-dev


I think it will be installed by the other plugin but might as well.

dragomirp · 2024-01-17T15:47:08Z

charmcraft.yaml

+    organize:
+      "*-linux-gnu/libpq.so*": binlibs/


We only want libpq SOs

dragomirp · 2024-01-17T15:49:32Z

src/charm.py

@@ -1,4 +1,4 @@
-#!/usr/bin/env python3
+#!/usr/bin/env -S LD_LIBRARY_PATH=binlibs python3


This is magical, but seems to also work in focal. Alternatively, we can try to override the hooks to also add this, but I'm not sure there is a clean way to do that either.

IMHO, it's cleaner to keep it here.

It might be cleaner to override dispatch (which overrides all the hooks)

You can grab the default dispatch from charmcraft's repo, override it, and charmcraft will use that instead

I don't think we should rely on the structure of the charmcraft repo. We won't notice if anything changes there.

dragomirp · 2024-01-17T15:50:30Z

tox.ini

@@ -69,7 +69,7 @@ commands =
 [testenv:unit]
 description = Run unit tests
 commands_pre =
-    poetry install --only main,charm-libs,unit --no-root
+    poetry install --only charm-libs,unit --no-root


Had issues installing psycopg3 locally for the unit tests, may end up not being necessary.

marceloneppel

The solution looks very clean. I liked it.

marceloneppel · 2024-01-18T20:49:21Z

src/charm.py

@@ -1,4 +1,4 @@
-#!/usr/bin/env python3
+#!/usr/bin/env -S LD_LIBRARY_PATH=binlibs python3


IMHO, it's cleaner to keep it here.

taurus-forever · 2024-01-22T08:30:20Z

In general LGTM, we can try LD_LIBRARY_PATH approach.
What is the building time difference?

dragomirp · 2024-01-22T10:36:23Z

What is the building time difference?

The build without cached psycopg on this PR took ~4 mins. I don't think it will be drastically affected.

dragomirp · 2024-01-27T18:20:09Z

charmcraft.yaml

+    organize:
+      "*-linux-gnu/libpq.so*": lib/


@delgod requested moving the SO to lib.

dragomirp · 2024-01-27T18:59:48Z

pyproject.toml

+[tool.poetry.group.cdeps]
+optional = true
+
+[tool.poetry.group.cdeps.dependencies]
+psycopg2 = "^2.9.9"
+psycopg = {extras = ["c"], version = "^3.1.17"}


If installing psycopg in the main group. It will be compiled at each unit test run and the unit tests won't be able to run on systems that do not have libpq installed.

marceloneppel

LGTM!

taurus-forever

Hey! This is superb!!! Well done!

carlcsaposs-canonical · 2024-01-30T08:08:36Z

pyproject.toml

+[tool.poetry.group.cdeps]
+optional = true
+
+[tool.poetry.group.cdeps.dependencies]
+psycopg2 = "^2.9.9"
+psycopg = {extras = ["c"], version = "^3.1.17"}


I don't this this group is necessary or should be added

Installing pyscopg2 without --no-binary (aka with poetry, during unit tests) works fine on Ubuntu

Installing pyscopg2 with --no-binary (aka with charmcraft pack), with the changes Dragomir made, works fine

So, it's possible to only have pyscopg2 and pyscopg in the main dependency group, remove psycopg2-binary from the unit dependency group, and remove the c dependency group entirely

Libpq was not present for sure on the self-hosted runner images nor is it a given that unit tests would be run on Ubuntu flavour that has libpq. Using compiled psycopg also means that CI unit tests would have to recompile on each run. @marceloneppel @taurus-forever what do you think?

unit tests would have to recompile on each run

wdym by recompile?

fwiw pip install pyscopg2 on ubuntu runs quite quickly (few seconds)—I believe it's building a wheel from source but it doesn't seem to be any slower than a normal pip install from a binary wheel download

In this version unit tests task passes in ~10s: https://github.com/canonical/pgbouncer-k8s-operator/actions/runs/7714293497/job/21026204594?pr=179
If using just psycopg2 from the main group unit tests task takes ~30s: https://github.com/canonical/pgbouncer-k8s-operator/actions/runs/7714361605/job/21026430075?pr=203

Considering that the difference is up to declaring dependencies differently, I think this is the better approach.

it seems like something weird is going on

poetry add pyscopg2 takes 8 seconds (including venv creation) on a fresh gh runner: https://github.com/canonical/pgbouncer-k8s-operator/actions/runs/7723418033/job/21053662267

that doesn't account for a >= 20 second difference

Just pip installing the packages has ~8s difference: https://github.com/canonical/pgbouncer-k8s-operator/actions/runs/7725456003/job/21059694185?pr=207

Removing pre-compiled binary here has a way bigger strategic effect comparing to 20 seconds saving.
BTW, 20 seconds is a noticeable time, we can safe, but as a separate PR IMHO.

Please, branch this to a new bugreport and unblock this huge merge please (as both options are safe and does the job). Tnx!

github-actions bot added the Libraries: Out of sync label Jan 16, 2024

dragomirp force-pushed the dpe-3062-binaries branch from ec52ae7 to a4d93f1 Compare January 16, 2024 21:21

Shebang magic and dump plugin

e859d71

dragomirp force-pushed the dpe-3062-binaries branch from a4d93f1 to e859d71 Compare January 16, 2024 21:28

dragomirp added 2 commits January 17, 2024 14:57

Remove usr/share

2290ec5

Don't download the deb directly

9fe0f84

dragomirp force-pushed the dpe-3062-binaries branch from 8613adb to 9fe0f84 Compare January 17, 2024 14:30

dragomirp commented Jan 17, 2024

View reviewed changes

Merge branch 'main' into dpe-3062-binaries

a2eb53c

marceloneppel reviewed Jan 18, 2024

View reviewed changes

dragomirp added 7 commits January 26, 2024 15:15

Merge branch 'main' into dpe-3062-binaries

107851c

Move binlibs into the lib dir

8fe03af

Try to revert tox

4e8c21f

Try to break unit tests

5cc185f

Split away cdeps

0085c81

Revert ci

973c45e

Merge branch 'main' into dpe-3062-binaries

d31d4f9

dragomirp changed the title ~~[TEST] Removing binary deps~~ [DPE-3062] Removing binary deps Jan 27, 2024

dragomirp commented Jan 27, 2024

View reviewed changes

dragomirp marked this pull request as ready for review January 27, 2024 18:59

dragomirp requested review from marceloneppel, taurus-forever and carlcsaposs-canonical January 27, 2024 19:00

marceloneppel approved these changes Jan 29, 2024

View reviewed changes

Merge branch 'main' into dpe-3062-binaries

b05dbfc

taurus-forever approved these changes Jan 29, 2024

View reviewed changes

Merge branch 'main' into dpe-3062-binaries

9943bec

carlcsaposs-canonical requested changes Jan 30, 2024

View reviewed changes

dragomirp added 6 commits January 30, 2024 18:03

Psycopg2 from source

f31d321

Merge branch 'main' into dpe-3062-binaries

4bd370b

Merge branch 'dpe-3062-binaries' into dpe-3062-binaries-source

3b00ff4

Remove extra dep group

0915ebd

Merge branch 'main' into dpe-3062-binaries

c2bc503

Bump libs

185b5af

github-actions bot added Libraries: OK and removed Libraries: Out of sync labels Jan 31, 2024

dragomirp requested a review from carlcsaposs-canonical January 31, 2024 18:09

carlcsaposs-canonical approved these changes Feb 5, 2024

View reviewed changes

Merge branch 'main' into dpe-3062-binaries

df84a20

marceloneppel approved these changes Feb 5, 2024

View reviewed changes

Merge branch 'main' into dpe-3062-binaries

7430b18

dragomirp merged commit c010b48 into main Feb 5, 2024
29 checks passed

dragomirp deleted the dpe-3062-binaries branch February 5, 2024 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DPE-3062] Removing binary deps #179

[DPE-3062] Removing binary deps #179

dragomirp commented Jan 16, 2024 •

edited

Loading

dragomirp Jan 17, 2024

dragomirp Jan 17, 2024

dragomirp Jan 17, 2024

marceloneppel Jan 18, 2024

carlcsaposs-canonical Jan 30, 2024

dragomirp Jan 30, 2024

dragomirp Jan 17, 2024

marceloneppel left a comment

marceloneppel Jan 18, 2024

taurus-forever commented Jan 22, 2024

dragomirp commented Jan 22, 2024

dragomirp Jan 27, 2024

dragomirp Jan 27, 2024

marceloneppel left a comment

taurus-forever left a comment

carlcsaposs-canonical Jan 30, 2024

dragomirp Jan 30, 2024

carlcsaposs-canonical Jan 30, 2024

dragomirp Jan 30, 2024

carlcsaposs-canonical Jan 31, 2024

dragomirp Jan 31, 2024

taurus-forever Jan 31, 2024

		@@ -1,4 +1,4 @@
		#!/usr/bin/env python3
		#!/usr/bin/env -S LD_LIBRARY_PATH=binlibs python3

[DPE-3062] Removing binary deps #179

[DPE-3062] Removing binary deps #179

Conversation

dragomirp commented Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marceloneppel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taurus-forever commented Jan 22, 2024

dragomirp commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marceloneppel left a comment

Choose a reason for hiding this comment

taurus-forever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dragomirp commented Jan 16, 2024 •

edited

Loading