Skip to content

Adding support for Flatpak#309

Merged
kwart merged 12 commits intointoolswetrust:masterfrom
Sesivany:master
Apr 2, 2026
Merged

Adding support for Flatpak#309
kwart merged 12 commits intointoolswetrust:masterfrom
Sesivany:master

Conversation

@Sesivany
Copy link
Copy Markdown
Contributor

Consider this as WIP, but I wanted to create a PR to move it further. I've created the /distribution/linux directory where I placed everything. Flatpak specific files are in /distribution/linux/flatpak.

There are currently two manifests: one builds the flatpak from the official binary, one from git repo source. Since Flathub requires a build from source if the source is available, I will base the version for Flathub on this. There will be some additions needed. Flathub requires reproducibility, it will have to point to an exact branch and commit. But it isn't very practical for upstream because upstream manifest should ideally build from the latest master.

I've also put the hires icon file in flatpak dir, let's use it for that only ATM.

I've also added several screenshots from Linux. Once they're accepted to the repo and I know their URLs on Github, I will update the metadata file to point to them.

The manifests currently assume that the new files which aren't in the repo are in the same dir as the manifest. It's a bit of an chicken-egg problem. Once they're accepted, I will rewrite the manifest to use them from the repo.

@Sesivany Sesivany mentioned this pull request Mar 19, 2026
@kwart kwart self-requested a review March 20, 2026 09:57
Copy link
Copy Markdown
Member

@kwart kwart left a comment

Choose a reason for hiding this comment

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

Thanks @Sesivany, for the feature PR.
I went through the review and have a few comments. Most of them are minor things - just URL updates. The important one is probably a missing INNER script.
Could you have a look?

Sesivany and others added 6 commits March 25, 2026 16:49
Co-authored-by: Josef Cacek <josef.cacek@gmail.com>
Co-authored-by: Josef Cacek <josef.cacek@gmail.com>
Co-authored-by: Josef Cacek <josef.cacek@gmail.com>
Co-authored-by: Josef Cacek <josef.cacek@gmail.com>
…files since they're already accepted upstream in a different path
@Sesivany
Copy link
Copy Markdown
Contributor Author

I've completely reworked the dep list generation. Now it is simplified into one script. I've also fixed paths to files such as the .desktop in the flatpak manifest and added screenshots to the appstream metadata file. The screenshot files are removed since they're already accepted in a different path. I've also added a missing jsignpdf-flatpak.in file where java opts are stored.

@kwart
Copy link
Copy Markdown
Member

kwart commented Mar 26, 2026

I didn't have time to look into it today. For now, just comments from Claude:


New review findings

Issues

1. Multiple type="default" screenshots in metainfo.xml (Medium)

File: distribution/linux/io.github.intoolswetrust.JSignPdf.metainfo.xml:42-55

All three <screenshot> elements have type="default". Per the AppStream specification, only one screenshot should be marked as type="default" (the primary/thumbnail screenshot). The rest should omit the type attribute entirely.

<!-- Current (all three are type="default") -->
<screenshot type="default">...</screenshot>
<screenshot type="default">...</screenshot>
<screenshot type="default">...</screenshot>

<!-- Should be -->
<screenshot type="default">...</screenshot>
<screenshot>...</screenshot>
<screenshot>...</screenshot>

2. License in metainfo.xml is incomplete (Low)

File: distribution/linux/io.github.intoolswetrust.JSignPdf.metainfo.xml:6

The metainfo declares <project_license>MPL-2.0</project_license>, but the project's pom.xml lists two licenses: MPL-2.0 and LGPL-2.1. The SPDX expression should be MPL-2.0 AND LGPL-2.1-only (or LGPL-2.1-or-later depending on intent) to accurately represent the licensing.

3. Binary manifest has incorrect relative paths (Low / by design?)

File: distribution/linux/flatpak/io.github.intoolswetrust.JSignPdf.yaml:30-34

The binary manifest references jsignpdf.desktop and io.github.intoolswetrust.JSignPdf.metainfo.xml without ../ prefix, but these files live in distribution/linux/, not distribution/linux/flatpak/. The Devel manifest correctly uses ../ prefix for these.

This may be intentional if the binary manifest is meant for the Flathub repo (where files would be co-located), as mentioned in the PR description. If so, a comment at the top of the file clarifying this would help.

4. generate-dependencies.sh - jq output formatting issue (Low)

File: distribution/linux/flatpak/generate-dependencies.sh:100-107

The JSON assembly logic has an inconsistency:

  • When jq is available (line 103): jq -s '.' slurps the entries into an array and outputs proper JSON. But then tail -n +2 | head -n -1 strips the outer [ and ], and the script adds its own [ and ] on lines 100 and 107. This works but produces double-indented output and the array already has proper commas from jq.
  • When jq is not available (line 105): sed '1s/^/ /; 1!s/^/, /' prepends commas to lines 2+, but the first line is not comma-prefixed. This means the first entry has no comma before it (correct) but subsequent entries have a leading comma instead of a trailing one. The result is technically valid JSON but unconventional formatting.

More importantly, the grep -c on line 112 searches for "type": "file" with spaces, but the entries are generated as compact JSON ("type":"file" without spaces). This means the count will always report 0 in the final summary message. Should match the actual format or use jq for counting.

5. Unquoted variables in heredoc (Low)

File: distribution/linux/flatpak/generate-dependencies.sh:59

git clone --depth 1 --branch ${BRANCH} ${REPO_URL} "\$BUILD_DIR"

${BRANCH} and ${REPO_URL} are expanded by the outer shell (before being passed to flatpak). While the defaults are safe, if a user overrides these env vars with values containing spaces, the command would break. Consider quoting: "${BRANCH}" "${REPO_URL}".

6. --share=network in finish-args (Note - probably fine)

Files: Both .yaml manifests

--share=network is included in the Flatpak finish-args. This is likely needed for TSA timestamping and CRL/OCSP revocation checks, which are core features. Just flagging for awareness since many Flatpak reviewers (especially Flathub) scrutinize network access.

Minor / cosmetic

  • jsignpdf.desktop:12 - file has a trailing blank line (line 12 is empty). Some linters flag this.
  • maven-dependencies.json - 4335 lines / 619 dependency entries. This is a large generated file. Consider adding it to .gitattributes with linguist-generated=true so it doesn't pollute diffs and code review.
  • Commit history - 10 commits including 4 sequential metainfo.xml updates via GitHub UI. Consider squashing before merge for a cleaner history.

Summary

The PR is in good shape after the latest round of fixes. The critical issue from the first review (missing inner script) has been resolved by reworking the dependency generation into a single self-contained script. All URL-related review comments have been addressed.

The remaining issues are mostly minor. The screenshot type="default" duplication (#1) and the grep -c pattern mismatch (#4) are the most actionable items. The license question (#2) is worth discussing but may be intentionally simplified.

@Sesivany
Copy link
Copy Markdown
Contributor Author

Sesivany commented Apr 1, 2026

I tried to accommodate the feedback. I considered dropping --share=network, but it's indeed necessary for TSA.

Copy link
Copy Markdown
Member

@kwart kwart left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes. LGTM 🙏

@kwart kwart merged commit 358f0e5 into intoolswetrust:master Apr 2, 2026
1 check passed
@kwart
Copy link
Copy Markdown
Member

kwart commented Apr 2, 2026

If the distribution/linux/flatpak/maven-dependencies.json needs to stay up-to-date, we'll probably need additional automation to regenerate it. 🤔 (GitHub action? or is it possible to solve this on the Flathub side?)

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