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

[Packaging] Only package qm files #431

Merged
merged 2 commits into from
Jun 20, 2023
Merged

Conversation

nephros
Copy link
Collaborator

@nephros nephros commented Jun 18, 2023

Before this change, Storeman rpms contained translations/README.md,
translations.pri, and .ts files, all of which are unnecessary in an
installed app.

@nephros
Copy link
Collaborator Author

nephros commented Jun 18, 2023

Sorry, this is against master - am I right in assuming I should rebase against devel?

@Olf0
Copy link
Member

Olf0 commented Jun 19, 2023

Before this change, Storeman rpms contained translations/README.md, translations.pri, and .ts files, all of which are unnecessary in an installed app.

Oops, good catch; thank you.
Seems I never thoroughly looked at rpm -ql harbour-storeman-*.rpm before.

Sorry, this is against master - am I right in assuming I should rebase against devel?

Yes, please.
For details see harbour-storeman/wiki/Git-commit-workflow.

May I also suggest to match against *.qm (instead of just *qm) to indicate that this is a file name extension and to use the longest generic substring of .qm files.

Before this change, Storeman rpms contained translations/README.md,
translations.pri, and .ts files, all of which are unnecessary in an
installed app.
Copy link
Member

@Olf0 Olf0 left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@Olf0 Olf0 merged commit 942f10f into storeman-developers:devel Jun 20, 2023
1 check passed
@nephros nephros deleted the packaging branch June 20, 2023 18:09
Olf0 added a commit that referenced this pull request Jul 29, 2023
* Add comment

* Document changelog format

* Omit apparently superfluous processing of `%{version}` for `qmake5` (#405)

* Add normative comment for `version` and `release` tags

* Fix grammar

* Shorten by one line

* Fix typo

* Post release version increase

* Add stub for v0.3.6

* Update README.md

* Cease pointing to OpenRepos for issues … (#428)

* Cease pointing to OpenRepos for issues …
  … plus some wording and formatting enhancements.

* Enhance wording

* Shorten wording (#429)

* Omit `test*_` tag for building (#430)

* [build-sfos4.2_latest.yml] Omit `test*_`

* [build-sfos4.2.yml] Omit `test*_`

* [build-sfos3.3.yml] Omit `test*_`

* [build-sfos3.3.yml] Remove empty line

* [build-sfos3.1.yml] Omit `test*_`

* [Packaging] Only package qm files (#431)

* [Packaging] Only package qm files
  Before this change, Storeman rpms contained translations/README.md,
translations.pri, and .ts files, all of which are unnecessary in an
installed app.

* Be more specific about the file ending

---------

Co-authored-by: nephros <nemo@pgxperiiia10>

* [harbour-storeman.changes] Start listing changes for v0.3.6

---------

Co-authored-by: Peter G <github@nephros.org>
Co-authored-by: nephros <nemo@pgxperiiia10>
Olf0 added a commit that referenced this pull request Jul 30, 2023
* Add comment

* Document changelog format

* Omit apparently superfluous processing of `%{version}` for `qmake5` (#405)

* Add normative comment for `version` and `release` tags

* Fix grammar

* Shorten by one line

* Fix typo

* Post release version increase

* Add stub for v0.3.6

* Update README.md

* Cease pointing to OpenRepos for issues plus some wording and formatting enhancements. (#428)

* Enhance wording

* Shorten wording (#429)

* Omit `test*_` tag for building (#430)

* [build-sfos4.2_latest.yml] Omit `test*_`

* [build-sfos4.2.yml] Omit `test*_`

* [build-sfos3.3.yml] Omit `test*_`

* [build-sfos3.3.yml] Remove empty line

* [build-sfos3.1.yml] Omit `test*_`

* [Packaging] Only package qm files (#431)

* [Packaging] Only package qm files
  Before this change, Storeman rpms contained translations/README.md,
translations.pri, and .ts files, all of which are unnecessary in an
installed app.

* Be more specific about the file ending

---------

Co-authored-by: nephros <nemo@pgxperiiia10>

* [harbour-storeman.changes] Start listing changes for v0.3.6

---------

Co-authored-by: Peter G <github@nephros.org>
Co-authored-by: nephros <nemo@pgxperiiia10>
Olf0 added a commit that referenced this pull request Jul 30, 2023
* Add comment

* Document changelog format

* Omit apparently superfluous processing of `%{version}` for `qmake5` (#405)

* Add normative comment for `version` and `release` tags

* Fix grammar

* Shorten by one line

* Fix typo

* Post release version increase

* Add stub for v0.3.6

* Update README.md

* Cease pointing to OpenRepos for issues plus some wording and formatting enhancements. (#428)

* Enhance wording

* Shorten wording (#429)

* Omit `test*_` tag for building (#430)

* [build-sfos4.2_latest.yml] Omit `test*_`

* [build-sfos4.2.yml] Omit `test*_`

* [build-sfos3.3.yml] Omit `test*_`

* [build-sfos3.3.yml] Remove empty line

* [build-sfos3.1.yml] Omit `test*_`

* [Packaging] Only package qm files (#431)

* [Packaging] Only package qm files
  Before this change, Storeman rpms contained translations/README.md,
translations.pri, and .ts files, all of which are unnecessary in an
installed app.

* Be more specific about the file ending

---------

Co-authored-by: nephros <nemo@pgxperiiia10>

* [harbour-storeman.changes] Start listing changes for v0.3.6

---------

Co-authored-by: Peter G <github@nephros.org>
Co-authored-by: nephros <nemo@pgxperiiia10>
Olf0 added a commit that referenced this pull request Jul 30, 2023
* Add comment

* Document changelog format

* Omit apparently superfluous processing of `%{version}` for `qmake5` (#405)

* Add normative comment for `version` and `release` tags

* Fix grammar

* Shorten by one line

* Fix typo

* Post release version increase

* Add stub for v0.3.6

* Update README.md

* Cease pointing to OpenRepos for issues plus some wording and formatting enhancements. (#428)

* Enhance wording

* Shorten wording (#429)

* Omit `test*_` tag for building (#430)

* [build-sfos4.2_latest.yml] Omit `test*_`

* [build-sfos4.2.yml] Omit `test*_`

* [build-sfos3.3.yml] Omit `test*_`

* [build-sfos3.3.yml] Remove empty line

* [build-sfos3.1.yml] Omit `test*_`

* [Packaging] Only package qm files (#431)

* [Packaging] Only package qm files
  Before this change, Storeman rpms contained translations/README.md,
translations.pri, and .ts files, all of which are unnecessary in an
installed app.

* Be more specific about the file ending

---------

Co-authored-by: nephros <nemo@pgxperiiia10>

* [harbour-storeman.changes] Start listing changes for v0.3.6

---------

Co-authored-by: Peter G <github@nephros.org>
Co-authored-by: nephros <nemo@pgxperiiia10>
@Olf0
Copy link
Member

Olf0 commented Jul 30, 2023

Oh, while this PR did sound reasonable and it reduced Storeman|s binary size by ~ 30%, it brought unexpected consequences: Instead of any translatable string, its ID is shown (e.g., orn-about, orn-description-full etc.). Bummer!

I am using en_GB, which should fall back to en.
Binaries for testing Storeman 0.3.6 can be obtained on Storeman's releases page at GitHub (compiled at GitHub) and the SFOS-OBS (which I used for testing).

Building the .qm files seems to work fine (as it always did, since I started maintaining Storeman).

@nephros, do you have an idea what went wrong?

P.S.: Something else caught my eye when looking through the compilation output:
harbour-storeman.aarch64: W: unstripped-binary-or-object /usr/bin/harbour-storeman
Should I set the -debug flag somewhere in the GitHub-CI-workflow definition (.yaml file)?
Oh, as I am using the Coderus' simple docker images for Storeman, I cannot set this here AFAICS; I would have to switch to the more sophisticated images, as used for compiling Patchmanager.
Never mind, as the binaries for distribution (installs by Storeman Installer and updates by Storeman) are built at the SFOS-OBS, where I (should) have the Debug flag switched on. Do you concur with these considerations?

@nephros
Copy link
Collaborator Author

nephros commented Jul 30, 2023

I think I have seen this when the translation is idbased, and the -en.ts file contains some but not all translated strings.

Anyway I can't imagine how leaving out the .ts and README files could cause that.

@nephros
Copy link
Collaborator Author

nephros commented Jul 30, 2023

P.S.: Something else caught my eye when looking through the compilation output: harbour-storeman.aarch64: W: unstripped-binary-or-object /usr/bin/harbour-storeman Should I set the -debug flag somewhere in the GitHub-CI-workflow definition (.yaml file)? Oh, as I am using the Coderus' simple docker images for Storeman, I cannot set this here AFAICS; I would have to switch to the more sophisticated images, as used for compiling Patchmanager. Never mind, as the binaries for distribution (installs by Storeman Installer and updates by Storeman) are built at the SFOS-OBS, where I (should) have the Debug flag switched on. Do you concur with these considerations?

Yes probably related to this line:

https://github.com/storeman-developers/harbour-storeman/blob/devel/harbour-storeman.pro#L22

Normally the Makefile should use install -s at some point, likely debug mode turns that off.

@nephros
Copy link
Collaborator Author

nephros commented Jul 30, 2023

One thing I notice is that if you do

git co devel
lupdate *.pro

lupdate finds lots of vanished(obsolete) entries. Not sure why.

@Olf0
Copy link
Member

Olf0 commented Jul 30, 2023

I think I have seen this when the translation is ID-based, and the -en.ts file contains some but not all translated strings.

Just to clarify: There are no strings at all, solely their IDs displayed in the current (preview) release of Storeman 0.3.6.

lupdate finds lots of vanished (obsolete) entries. Not sure why.

May "lots of" be equivalent to "all" here? ;-)

@Olf0
Copy link
Member

Olf0 commented Jul 30, 2023

Before this change, Storeman rpms contained translations/README.md, translations.pri, and .ts files, all of which are unnecessary in an installed app.

I dunno about the exact compilation order, but maybe the .pri file is removed too early for this include statement?

@nephros
Copy link
Collaborator Author

nephros commented Jul 31, 2023

May "lots of" be equivalent to "all" here? ;-)

Nope, it's 40 translated, about 170 or so obsolete. But it may be my invocation of lupdate is wrong.

@nephros
Copy link
Collaborator Author

nephros commented Jul 31, 2023

Oh! OH! Simple.

The packages don't contain any .qm files at all! (At least https://repo.sailfishos.org/obs/home:/olf:/harbour-storeman:/testing/sailfish_testing_aarch64/aarch64/harbour-storeman-0.3.6-1.3.1.jolla.aarch64.rpm does not).

Now this would explain

[...] this PR [...] reduced Storeman|s binary size by ~ 30%

and of course

Instead of any translatable string, its ID is shown (e.g., orn-about, orn-description-full etc.). Bummer!

... exactly what happens if there is not translation (not a non-translated string, but no file to load it from!).

I have to look into how to fix this, it would seem the order of things (create .qm files, deploy them to install cache) is not correct.

@nephros
Copy link
Collaborator Author

nephros commented Jul 31, 2023

Oh, and I just discovered I have direct commit access here! Sorry I commited directly to devel and not this branch.

Lets see if 92f410f fixes the issue.

EDIT: it did not. Three commits later situation has not improved - I'll continue experimenting in a feature branch and come back later.

EDIT2: Ah wrong, the last version works in packaging the .qm files.

Issue solved? :)

@Olf0
Copy link
Member

Olf0 commented Jul 31, 2023

One thing I notice is that if you do
git co devel
lupdate *.pro
lupdate finds lots of vanished(obsolete) entries. Not sure why.

May "lots of" be equivalent to "all" here? ;-)

Nope, it's 40 translated, about 170 or so obsolete. But it may be my invocation of lupdate is wrong.

Note that I have never used lupdate and / or lrelease: I performed the lupdate part manually for the few strings I changed, the lrelease part is covered by Transifex's mechanisms. Maybe I did or configured something wrong.

The only thing I can contribute to that is the translation documentation I once overhauled.

@nephros
Copy link
Collaborator Author

nephros commented Jul 31, 2023

Nah, it's alright, not all the source files are picked up when using the .pro file as input - when using recursive mode all strings are found.

nemo@PGXperiiia10:~/git/_forks/storeman $ lupdate *pro -ts translations/harbour-storeman.ts
WARNING: Project ERROR: Unknown module(s) in QT: core gui concurrent dbus core-private
lupdate warning: TS files from command line will override TRANSLATIONS in /home/nemo/devel/git/_forks/storeman/harbo
ur-storeman.pro.
Updating 'translations/harbour-storeman.ts'...
Found 40 source text(s) (0 new and 40 already existing)
Kept 219 obsolete entries
nemo@PGXperiiia10:~/git/_forks/storeman $ git restore translations/*.ts
nemo@PGXperiiia10:~/git/_forks/storeman $ lupdate qml/ src/ *top  -ts translations/harbour-storeman.ts
Scanning directory 'qml/'...
Scanning directory 'src/'...
/home/nemo/devel/git/_forks/storeman/qml/pages/SettingsPage.qml:97: Unterminated meta string
Updating 'translations/harbour-storeman.ts'...
Found 259 source text(s) (0 new and 259 already existing)

@Olf0
Copy link
Member

Olf0 commented Aug 3, 2023

Oh, and I just discovered I have direct commit access here!

Yes, as there is nobody SFOS-related who I trust as much as you: Because Mentaljam (Petr T.) is very hard to contact (this may fail for weeks) and does not monitor these repos actively, what will happen if I get hit by a car? I thought I once communicated that you have admin rights for all repos of the org storeman-developers (and IIRC the org itself), plus to the Storeman related OBS-repos under my account at the SFOS-OBS. Hence in case of an emergency (i.e., new SFOS release breaks Storeman and I am not at all available), you may handle that, if you want to (without Storeman, OpenRepos is inaccessible for most SFOS-users).

Sorry I commited directly to devel and not this branch.

Never mind: This is why I like the devel / master separation. master should always build correctly and its builds should always work at least O.K-ish. To ensure that, devel is the staging branch for master: If anything goes wrong, it can be easily reverted in devel, before it reaches the master branch.

Lets see if 92f410f fixes the issue.

Wow, Intel made really cool ads ... 13 years ago. I suppose this advertisement was created in the context of recruiting ... well, I can tell Intel was not a really cool place to be hired at even 13 years ago. But back then they were still mostly engineering driven, attracted lots of smart people due to high wages, but already exhibited most downsides of big corporations.

@Olf0
Copy link
Member

Olf0 commented Aug 3, 2023

Issue solved? :)

I will check in practice (with a generated RPM file) ... when I find some time for that.

Thank you very much, as I strongly assume that the altered line
translations.path = $$PREFIX/share/$$TARGET
translations.path = $$PREFIX/share/$$TARGET/translations plus the added line
translations.CONFIG += no_check_exist
in translations.pri fix the issue. (Why? See my first sentence in the previous message.)

Additional kudos for spotting the unterminated source string (though it apparently did no harm)!

Olf0 added a commit that referenced this pull request Aug 3, 2023
Olf0 added a commit that referenced this pull request Aug 3, 2023
… which had been [inserted prematurely](#431 (comment)).
Olf0 added a commit that referenced this pull request Aug 3, 2023
* [harbour-storeman.spec] Prepare another release for 0.3.6

Due to [this mishap](#431 (comment)).

* [harbour-storeman.changes] Remove stub for 0.3.7

… which had been [inserted prematurely](#431 (comment)).
Olf0 added a commit that referenced this pull request Aug 4, 2023
* [harbour-storeman.spec] Post release version increase

* [harbour-storeman.changes] Add stub for v0.3.7

* [harbour-storeman.spec] Rectify referenced line number

* Fix translation file install target

https://www.youtube.com/watch?v=IwfcKFrjCdg

* Fix translation output file path

* Fix up translation file generation

* Fix unterminated source string

* [translations/README.md] Indicate to use `lupdate`'s "recursive mode"

* Prepare another release for 0.3.6 (#437)

* [harbour-storeman.spec] Prepare another release for 0.3.6

Due to [this mishap](#431 (comment)).

* [harbour-storeman.changes] Remove stub for 0.3.7

… which had been [inserted prematurely](#431 (comment)).

---------

Co-authored-by: Peter G <sailfish@nephros.org>
Co-authored-by: nephros <nemo@pgxperiiia10>
Olf0 added a commit that referenced this pull request Aug 4, 2023
* [harbour-storeman.spec] Post release version increase

* [harbour-storeman.changes] Add stub for v0.3.7

* [harbour-storeman.spec] Rectify referenced line number

* Fix translation file install target

https://www.youtube.com/watch?v=IwfcKFrjCdg

* Fix translation output file path

* Fix up translation file generation

* Fix unterminated source string

* [translations/README.md] Indicate to use `lupdate`'s "recursive mode"

* Prepare another release for 0.3.6 (#437)

* [harbour-storeman.spec] Prepare another release for 0.3.6

Due to [this mishap](#431 (comment)).

* [harbour-storeman.changes] Remove stub for 0.3.7

… which had been [inserted prematurely](#431 (comment)).

---------

Co-authored-by: Peter G <sailfish@nephros.org>
Co-authored-by: nephros <nemo@pgxperiiia10>
Olf0 added a commit that referenced this pull request Aug 4, 2023
`master`→`sfos3.2`: Commits for 0.3.6-release2 (#438) (#439)
* [harbour-storeman.spec] Post release version increase

* [harbour-storeman.changes] Add stub for v0.3.7

* [harbour-storeman.spec] Rectify referenced line number

* Fix translation file install target

https://www.youtube.com/watch?v=IwfcKFrjCdg

* Fix translation output file path

* Fix up translation file generation

* Fix unterminated source string

* [translations/README.md] Indicate to use `lupdate`'s "recursive mode"

* Prepare another release for 0.3.6 (#437)

* [harbour-storeman.spec] Prepare another release for 0.3.6

Due to [this mishap](#431 (comment)).

* [harbour-storeman.changes] Remove stub for 0.3.7

… which had been [inserted prematurely](#431 (comment)).

---------

Co-authored-by: Peter G <sailfish@nephros.org>
Co-authored-by: nephros <nemo@pgxperiiia10>
Olf0 added a commit that referenced this pull request Aug 4, 2023
* [harbour-storeman.spec] Post release version increase

* [harbour-storeman.changes] Add stub for v0.3.7

* [harbour-storeman.spec] Rectify referenced line number

* Fix translation file install target

https://www.youtube.com/watch?v=IwfcKFrjCdg

* Fix translation output file path

* Fix up translation file generation

* Fix unterminated source string

* [translations/README.md] Indicate to use `lupdate`'s "recursive mode"

* Prepare another release for 0.3.6 (#437)

* [harbour-storeman.spec] Prepare another release for 0.3.6

Due to [this mishap](#431 (comment)).

* [harbour-storeman.changes] Remove stub for 0.3.7

… which had been [inserted prematurely](#431 (comment)).

---------

Co-authored-by: Peter G <sailfish@nephros.org>
Co-authored-by: nephros <nemo@pgxperiiia10>
@Olf0
Copy link
Member

Olf0 commented Aug 4, 2023

Issue solved? :)

I will check in practice (with a generated RPM file) ... when I find some time for that.

Yes, this is working fine, as the results of the CI runs for merging the master branch into the sfos3.2 branch (built on the SFOS3.2 SDK), the sfos3.3 branch (built on the SFOS3.3 SDK), the sfos4.2 branch (built on the SFOS4.2 SDK) and again the sfos4.2 branch (but built on the SFOS-latest SDK, i.e., 4.5) show.

Thank you very much!

P.S.: Still I have two minor questions left, which I will pose after the weekend.

@Olf0
Copy link
Member

Olf0 commented Aug 5, 2023

P.S.: Still I have two minor questions left, which I will pose after the weekend.

I was able to resolve them on my own:

1. no_check_exist

the altered line
translations.path = $$PREFIX/share/$$TARGET
translations.path = $$PREFIX/share/$$TARGET/translations plus the added line
translations.CONFIG += no_check_exist
in translations.pri fix the issue.

"no_check_exist" sounded unlogical to me, because why would one not want to check if they already exist (and hence do not need to be build)? And how does that possibly solve the issue, because in any case this would either leave files in place, which already existed or overwrite them, practically yielding the same!?!

Well, looking no_check_exist up shows that this flag is misleadingly named:
If not set, qmake looks to see if the files to install actually exist. If these files don't exist, qmake doesn’t create the install rule. Use this config value if you need to install files that are generated as part of your build process, like HTML files created by qdoc.

O.K., setting no_check_exist now completely makes sense, as did adapting the path, after the initial change selected only files in the translations directory, but not any longer the complete directory.

2. Warning: unstripped-binary

I do not comprehend your reply, and as usual my initial motion was to ultimately understand it, so I searched the WWW, which did not yield anything I recognised as useful, and was about to ask you.

But thinking about it, it is absolutely fine for the built binaries at GitHub to include debuginfo: They are only meant for testing and debugging. The distributed versions of Storeman (i.e., the ones installed by Storeman Installer and Storeman's self-updating mechanism from the Sailfish-OBS) are configured to create separate debuginfo files.

@Olf0
Copy link
Member

Olf0 commented Aug 16, 2023

Summary

Thanks a lot @nephros for starting this PR, implementing and fixing it, because despite taking a meandering, a bit tedious route, this enhancement ultimately reduced the size of the binaries significantly: By 87 KiB, which is almost 20%! 🥳

Additionally it allowed to check in practice, if the current tag naming and versioning scheme is sound, both at GitHub and the SFOS-OBS, specifically for fix-up releases which only increase the release version (never had this case, since I employed the current scheme): It is. 🙂

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

2 participants