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

FHS fixes #5930

Merged
merged 1 commit into from Jul 9, 2021
Merged

FHS fixes #5930

merged 1 commit into from Jul 9, 2021

Conversation

dimitry-ishenko
Copy link
Contributor

These patches:

  1. Fix some of the install dirs for the FHS version of the package.
  2. Add debian dir to help with building native .deb package for Debian-based distros.

@pmjdebruijn
Copy link
Contributor

I've tried 691ab92 and it seems fine.

@pmjdebruijn
Copy link
Contributor

I just noticed 691ab92 excludes the local font(s), which means some parts of the UI fall back to some monospaced font.

@dimitry-ishenko
Copy link
Contributor Author

@pmjdebruijn it's unusual for packages to bundle their own fonts (at least on Linux). My other patch b5d5386 adds dependencies on fonts-noto and fonts-noto-cjk.

@dimitry-ishenko
Copy link
Contributor Author

dimitry-ishenko commented Feb 28, 2021

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.

@pmjdebruijn
Copy link
Contributor

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).

@dimitry-ishenko
Copy link
Contributor Author

@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.

@pmjdebruijn
Copy link
Contributor

@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
/usr/share/icons/hicolor/128x128/apps/PrusaSlicer.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.

@dimitry-ishenko
Copy link
Contributor Author

dimitry-ishenko commented Mar 4, 2021

@dimitry-ishenko I don't work for prusa3d though... I'm merely pointing out stuff I'd imagine they would...

Oh I see. Well, I appreciate that.

However I noticed icons were being placed directly in /usr/share/icons, which is a bit nonstandard...

You are right. Sorry, I've missed. Will get it fixed.

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.

Makes sense. Will make a separate PR for this.

WRT the fonts, they shouldn't be stripped at all, they are required for prusa-slicer to work as intended...

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
Copy link
Contributor

@bubnikv I've tested d93dc28, please consider merging this...

@pmjdebruijn
Copy link
Contributor

11edda2 is just a rebase, practically identical to d93dc28 which I tested earlier...

@dimitry-ishenko
Copy link
Contributor Author

@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.

@pmjdebruijn
Copy link
Contributor

@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 ...

@bubnikv
Copy link
Collaborator

@tamasmeszaros would you please look into that?

@dimitry-ishenko
Copy link
Contributor Author

@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.
@dimitry-ishenko
Copy link
Contributor Author

@tamasmeszaros @bubnikv ping

@dimitry-ishenko
Copy link
Contributor Author

@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?

@bubnikv bubnikv merged commit 454b876 into prusa3d:master Jul 9, 2021
@bubnikv
Copy link
Collaborator

Sorry for the delay. We are too busy, there is always something.
Thanks for contribution.

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.

None yet

3 participants