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

dev_scripts: Handle Dangerzone packages with patch level != 1 #882

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

apyrgio
Copy link
Contributor

@apyrgio apyrgio commented Jul 30, 2024

Update our env.py script to auto-detect the correct Dangerzone package name. This is useful when building an end-user environment, i.e., a container image where we copy the respective Dangerzone .deb/.rpm package and install it via a package manager.

To achieve this, we replace the hardcoded patch level (-1) in the package name with a glob character (*). Then, we check in the respective build directory if there's exactly one match for this pattern. If yes, we return the full path. If not, we raise an exception.

Note that this limitation was triggered when we were building RPM packages for the 0.7.0 hotfix release.

Refs #880

Update our `env.py` script to auto-detect the correct Dangerzone package
name. This is useful when building an end-user environment, i.e., a
container image where we copy the respective Dangerzone .deb/.rpm
package and install it via a package manager.

To achieve this, we replace the hardcoded patch level (`-1`) in the
package name with a glob character (`*`). Then, we check in the
respective build directory if there's exactly one match for this
pattern. If yes, we return the full path. If not, we raise an exception.

Note that this limitation was triggered when we were building RPM
packages for the 0.7.0 hotfix release.

Refs #880
@rocodes rocodes self-assigned this Jul 31, 2024
@rocodes rocodes self-requested a review July 31, 2024 12:35
Copy link
Contributor

@rocodes rocodes left a comment

Choose a reason for hiding this comment

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

Thanks @apyrgio, took a look through this PR and also familiarized myself a bit more with the dev_scripts. The README is great and made all this accessible for someone who's still getting up to speed with the repo otherwise, so thank you.

AIUI this is all bootstrapping a testing environment, and that's all it's used for, so with that scope in mind, and keeping in mind that it unblocks a hotfix, I'm going to approve. But I think there might be room for some refactoring/simplifying here at some point. It was a bit surprising to me to read

There are times when we don't know the exact name of the Dangerzone package we've built

I think I understand why (in this case bumping the Release version in the spec file is distinct from the version.txt; also, aiui the dev env is not restricted to the most recent release, since it accepts a version param as a CLI arg), but it's a little counterintuitive. I didn't have enough time to see if there could also be hard-to-debug behaviour and if there was a more straightforward way for this to work (without inconveniencing devs and making the cli args more annoying that is), but the gears are turning. :)

@rocodes
Copy link
Contributor

rocodes commented Jul 31, 2024

An additional comment: When reviewing freedomofpress/yum-tools-prod#25, I understood that this wasn't just a failing CI check that was blocking package builds, but that it's a preflight release check for built artifacts as well.

https://github.com/freedomofpress/yum-tools-prod/blob/97aed85442670aa4e91b504631c1f2f0a279b105/.github/workflows/check-packages.yml#L49-L52

In that case, I would say that we should clarify that it's also used there, and I would be less inclined to be lax about version globbing - I think it's a fair restriction for pre-deployment CI to require an exact package version match somewhere. But it's not something I want to block anyone on for this specific release.

@apyrgio
Copy link
Contributor Author

apyrgio commented Jul 31, 2024

Fair point, I was debating whether we should a --patch-level argument as well. I'll open an issue for this, and we can reconsider this stance. For the time being, I'll merge this PR to unblock the yum-tools-prod one.

Thanks a lot for the thorough review, and I'd be happy to hear how we can improve this (admittedly overly complex) script.

@apyrgio apyrgio merged commit c1dbe9c into main Jul 31, 2024
49 checks passed
@apyrgio apyrgio deleted the 2024-07-patch-level branch July 31, 2024 14:40
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