-
Notifications
You must be signed in to change notification settings - Fork 2k
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
FHS fixes #5930
FHS fixes #5930
Conversation
I've tried 691ab92 and it seems fine. |
I just noticed 691ab92 excludes the local font(s), which means some parts of the UI fall back to some monospaced font. |
@pmjdebruijn it's unusual for packages to bundle their own fonts (at least on Linux). My other patch b5d5386 adds dependencies on |
Please note these patches only apply to the FHS version and don't affect Windows or standalone Flatpack versions. If you want, I can still leave the fonts in and instead strip them out inside the debian package. |
I'm not sure if Prusa is going to keep distro specific package upstream... You may want to consider splitting this up, so at least the FHS changes have a chance of getting merged if you remove the stripping of the font(s). |
@pmjdebruijn I've fixed the patch (moved font stripping into debian package). Would also like to ask you to consider including my other patch. I understand you not wanting to include distro-related stuff and that makes sense. However, Debian-based systems account for more than half of all Linux installs. Having the debian directory does not affect any other functionality, but will make it much easier for people to build and install this package. |
@dimitry-ishenko I don't work for prusa3d though... I'm merely pointing out stuff I'd imagine they would... I've tested b4ad4fb and it works fine, practically speaking... However I noticed icons were being placed directly in /usr/share/icons, which is a bit nonstandard, something like below would be better if I'm not mistaken: /usr/share/icons/hicolor/128x128/apps/PrusaSlicer-gcodeviewer.png With regard to the debian packaging, I would suggest dropping that from this pull request, so it's the FHS fixes only, and considering resubmitting the debian packaging once the FHS fixes have been pulled. WRT the fonts, they shouldn't be stripped at all, they are required for prusa-slicer to work as intended, the real debian package solves this by symlinking the ttf files to system copies, and as I mentioned before hard depending on the fonts-noto, fonts-noto-cjk packages. |
Oh I see. Well, I appreciate that.
You are right. Sorry, I've missed. Will get it fixed.
Makes sense. Will make a separate PR for this.
I disagree with you on this point. This might make sense for a Flatpak, but not for a regular package. If every app bundled its own fonts, we would have a mess. It's not entirely unlike shared libraries that the app depends on (eg, wxWidgets, GTK3, etc.). They don't get bundled. It's expected that they are available at runtime. Fonts are actually in a better position than shared libs, thanks to fontconfig whose job is to provide a reasonable substitute if the requested font is missing. With all that said, I did leave them in and added a patch to the debian rules to strip them (and instead added dependency on noto packages). |
@pmjdebruijn sorry, I didn't realize this PR was still open. It is identical to the original one. I was just trying (and failing) to compile 2.4.0-alpha last night and rebased it a few times. |
@bubnikv is there anything you need from us, for this to be considered to be merged? it would be rather nice to have in 2.3.1 ... |
@tamasmeszaros would you please look into that? |
@tamasmeszaros anything I can do to help get this merged? It's a pretty trivial patch. Sadly, 2.3.1 has already been released. @bubnikv any plans for 2.3.2? |
Install .desktop files into /usr/share/applications. Install PNGs for the above into /usr/share/icons. Install udev rules into /lib/udev/rules.d.
@tamasmeszaros @bubnikv ping |
@tamasmeszaros @bubnikv @pmjdebruijn hi guys, I hate to be a pest, but this seems like a very harmless patch, but it's been rotting for over half a year now. Could you please review and accept? |
Sorry for the delay. We are too busy, there is always something. |
These patches: