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

feat(bundler): Add RPM packaging, closes #4402 #5202

Merged
merged 3 commits into from Dec 23, 2023

Conversation

olivierlemasle
Copy link
Contributor

@olivierlemasle olivierlemasle commented Sep 15, 2022

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

  • This solution is pure-rust; it does not depend on rpmbuild, gpg2 or any other external tool.
  • I'm using the crate rpm-rs to build the RPM package. This crate is currently not actively maintained, but a community fork has been created to maintain it in the future. The forked crate, rpm, is now maintained by a community.
  • I'll try to backport this work to cargo-bundle (cf Add rpm target support burtonageo/cargo-bundle#115)

Status

  • Initial RPM packaging
  • Provides: and Requires:
  • Packaging of external binaries and resources
  • Generate and install .desktop file (shared with .deb)
  • Install icons (shared with .deb)
  • Install custom files
  • Owned directories
  • PGP signature
  • User documentation

Closes #4402

@PyRo1121
Copy link

PyRo1121 commented Nov 2, 2022

Are there any updates on this PR? Just checking to see what the current status is and if any help is needed.

@olivierlemasle
Copy link
Contributor Author

@PyRo1121 Thank you for your message.

Actually, I was waiting for 2 two things before completing this work:

  1. Clarification about the future or the rpm library I used (rpm-rs). As I said above, a fork of rpm-rs recently happened to continue the development of this library. There was some questions regarding the crate name. It has recently been resolved: the fork will use the name rpm; a new release has yet to be released.
  2. Early feedback from Tauri team (This is still Work-in-progress as stated above, but I'd like to know if this may be "mergeable" in the future, before completing it. Perhaps @amrbashir @lucasfernog ?)

I just rebased the PR to fix conflicts.

@FabianLars
Copy link
Member

but I'd like to know if this may be "mergeable" in the future, before completing it.

Yes! I was/We were primarily waiting for you to consider this PR to be ready for review (besides being busy with other stuff) :)

If you think it's ready for a first round of reviews then i'll add it to my todo list and i'm sure one of the others would take a look as well :)

P.S. thank you so much for your contribution to Tauri ❤️

@olivierlemasle
Copy link
Contributor Author

Thank you @FabianLars for your kind response.
I'll ping you when it's ready 🧑‍🍳

@Apogeum12
Copy link
Sponsor

What is status this feature? I'm hope then that will be fast into tauri
@olivierlemasle very good work, thanks!

@olivierlemasle
Copy link
Contributor Author

Hi @Apogeum12, I've just rebased the PR and updated it to use the crate rpm (fork of unmaintained rpm-rs), which was just released in alpha version last week.

I should now be able to fix the remaining missing points (cf initial comment).

@devpikachu
Copy link

Hello @olivierlemasle ! Thanks for your work on bringing this feature into Tauri. :) Do you currently have any approximate ETA for when this PR will be review-ready?

@olivierlemasle olivierlemasle marked this pull request as ready for review October 1, 2023 19:52
@olivierlemasle olivierlemasle requested a review from a team as a code owner October 1, 2023 19:52
@olivierlemasle
Copy link
Contributor Author

@devpikachu @FabianLars Sorry for the delay, I've just rebased and updated the PR, which is now ready for review.

@0-don
Copy link

0-don commented Oct 12, 2023

this would be really nice, good work olivierlemasle

@FabianLars
Copy link
Member

Just so you know we're not ignoring you, i will take a look next week when we're back from EuroRust. But from what i've seen so far, awesome work!!

Copy link
Member

@FabianLars FabianLars left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay.

All in all it looks good to me. Api/Config wise i think it's good to go.

The only question i have is about the dependencies notation. Is the .so and bits stuff really required? Shouldn't just webkit2gtk4.1 for example work just as well? Or are there contexts where this won't work, or some general guidelines/best practices i'm not aware of?

@0-don
Copy link

0-don commented Dec 11, 2023

so whats the exact problem currently @olivierlemasle

@olivierlemasle
Copy link
Contributor Author

Thank you @FabianLars for the review
Thanks @don-cryptus for the ping - I actually missed the notification and did not see @FabianLars's review

Regarding #8055, @FabianLars do you confirm you prefer merging this PR on RPM, before working on bundle configuration refactoring?

Regarding dependency configuration: I used the .so notation to try to be independent of the Linux distribution:

For example, on Fedora 37 and later, "webkit2gtk 4.1" is provided by a package named webkit2gtk4.1, which "provides":

  • libwebkit2gtk-4.1.so.0()(64bit)
  • webkit2gtk4.1
  • webkit2gtk4.1(x86-64)

On openSUSE Leap 15, "webkit2gtk 4.1" is provided by a package named libwebkit2gtk-4_1-0, which "provides":

  • WebKitGTK-4.1
  • libwebkit2gtk-4.1.so.0()(64bit)
  • libwebkit2gtk-4_1-0
  • libwebkit2gtk-4_1-0(x86-64)
  • libwebkit2gtk3

If the RPM contains a dependency to webkit2gtk4.1, it won't be possible to install it on openSUSE Leap.

It's the same problem for the other dependencies:

  • libgtk-3.so.0()(64bit) is provided by gtk3 on Fedora and by libgtk-3-0 on openSUSE.
  • libappindicator3.so.1()(64bit) is provided by libappindicator-gtk3 on Fedora and by libappindicator3-1 on openSUSE.

A better solution may exist, but using the .so notation seems to do the job. I must add that I do not know openSUSE well. I also did not test with other RPM-based distributions, but I think it should work (e.g. according to this page, libwebkit2gtk-4.1.so.0()(64bit) is also provided on OpenMandriva and Mageia by packages with another name: lib64webkit2gtk4.1)

@olivierlemasle
Copy link
Contributor Author

I've just rebased on latest dev to fix some conflicts.

@olivierlemasle
Copy link
Contributor Author

Also updated crate rpm for some improvements, including:

  • compatibility with passphrase-protected GPG keys
  • use of a package long description (different from the summary)

@FabianLars
Copy link
Member

Regarding #8055, @FabianLars do you confirm you prefer merging this PR on RPM, before working on bundle configuration refactoring?

I don't really care, it's mostly just to make it easier for you and to get this PR merged quicker. Though looking at all the open PRs we should probably merge some more stuff before the refactor to not cause tons of conflicts 😮‍💨

Regarding dependency configuration: I used the .so notation to try to be independent of the Linux distribution:

Thanks for the explanation. I guess i messed up big time because i checked that before asking the question but now i see opensuse being different too so i wonder what the hell i compared fedora too 😂


Anyway, with that done i don't see any blockers here so i'll do another test tomorrow-ish and then it'd be good to go for me. Thanks again for your work :)

@FabianLars FabianLars added the security: needs audit This issue/PR needs a security audit label Dec 19, 2023
@tweidinger
Copy link
Contributor

Had a quick look (at the PR and the RPM spec) and must admit I am not too familiar with rpm packaging.

@FabianLars could you package an example application and verify with rpmlint if the generated rpm does not violate best practices?

Otherwise I think this is a nice addition to the packaging 🤩

@tweidinger tweidinger added security: feedback loop wg-security added some feedback but no conclusion based on feedbac and removed security: needs audit This issue/PR needs a security audit labels Dec 20, 2023
@FabianLars
Copy link
Member

rpmlint only throws error for missing a changelog and a buildhost tag. idk how we'd want to solve this, via simple tauri.conf properties? We don't have any changelog handling anywhere so idk, and a missing buildhost tag almost sounds like a feature to me so we probably don't want to automatically read this from the buildsystem...
-> imo it's fine fine to merge it with those 2 errors.

@olivierlemasle
Copy link
Contributor Author

FYI, on a simple demo project, rpmlint gives:

demo-alpha.x86_64: W: only-non-binary-in-usr-lib
demo-alpha.x86_64: W: no-url-tag
demo-alpha.x86_64: W: no-manual-page-for-binary demo-alpha
demo-alpha.x86_64: W: no-manual-page-for-binary demo-sidecar
demo-alpha.x86_64: W: no-documentation
demo-alpha.x86_64: E: no-changelogname-tag
demo-alpha.x86_64: E: no-buildhost-tag

On the same project, for the .deb package, lintian gives:

E: demo-alpha: copyright-file-contains-full-apache-2-license
E: demo-alpha: copyright-not-using-common-license-for-apache2
E: demo-alpha: missing-dependency-on-libc needed by usr/bin/demo-alpha and 1 others
E: demo-alpha: no-changelog usr/share/doc/demo-alpha/changelog.gz (native package)
W: demo-alpha: copyright-without-copyright-notice
W: demo-alpha: no-manual-page [usr/bin/demo-alpha]
W: demo-alpha: no-manual-page [usr/bin/demo-sidecar]
W: demo-alpha: recommended-field demo-alpha_0.0.2_amd64.deb Section

There's the same error regarding the absence of changelog. I guess we could indeed add in the future a changelog in tauri.conf (probably common to .deb / .rpm and perhaps other packaging formats).

Regarding the Buildhost tag, I also don't like reading the hostname of the build machine automatically. However, it is possible to add it as an opt-in feature. E.g. in tauri.conf either specify a "static" Buildhost tag (or its absence), or specify to use the hostname (e.g. with gethostname). Either way, I don't think it's blocker.

@hjmallon
Copy link

Adding a changelog feature which is generic and also makes rpmlint happy might be more trouble than it is worth. RPM changelogs have a standard format with dates and versions in them and it is unlikely more generic changelogs would use this layout.

@tweidinger tweidinger added security: reviewed This issue/PR has been review by wg-security and removed security: feedback loop wg-security added some feedback but no conclusion based on feedbac labels Dec 21, 2023
@FabianLars FabianLars merged commit 091100a into tauri-apps:dev Dec 23, 2023
33 checks passed
hjmallon pushed a commit to hjmallon/tauri that referenced this pull request May 17, 2024
…5202)

* feat(bundler): Add RPM packaging

* feat(bundler): Update 'rpm' to 0.13.1

* Fix fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security: reviewed This issue/PR has been review by wg-security
Projects
Status: 🔎 In audit
Development

Successfully merging this pull request may close these issues.

[feat] Bundle Tauri apps as RPM
8 participants