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

Run lrelease during cmake build instead of pre-build #3009

Merged
merged 2 commits into from
Mar 1, 2022

Conversation

SunderB
Copy link
Contributor

@SunderB SunderB commented Feb 9, 2022

This PR moves the step which runs lrelease on the .ts files into the CMakeList, which runs it at build time.
This needs testing to see if it works fine on Qt6.

This is done using the Qt Linguist Tools cmake functions.

In Qt6 there's qt_add_lrelease which runs lrelease and sets the files as dependencies to the target: https://doc-snapshots.qt.io/qt6-6.3/qtlinguist-cmake-qt-add-lrelease.html

In Qt5 there's qt5_add_translation which converts .ts to .qm files, but the qm files then have to be manually added as dependencies to the target: https://doc.qt.io/qt-5/qtlinguist-cmake-qt5-add-translation.html

@samaaron
Copy link
Collaborator

Seems like a very sensible move. @cmaughan - do you have any comments on this?

@lilyinstarlight
Copy link
Contributor

I also think doing this is a good idea, and will try to test it soon with Qt5 on my system (NixOS). @SunderB, would you be able to rebase these changes again since #3018 has been merged and conflicts with this branch?

@cmaughan
Copy link
Contributor

Assuming this only happens once, it is fine; I thought it was in the prebuild because it was happening all the time.

@samaaron
Copy link
Collaborator

@cmaughan - excellent point. We don't necessarily want to run lrelease every time we do a build of the GUI when we're in tight dev cycles.

@cmaughan
Copy link
Contributor

cmaughan commented Feb 21, 2022

tight dev cycles or not, if it adds any unnecessary build time, then it reduces speed of light, as we say here. You pay that every time you fire up the build, which isn't acceptable. But it is OK if handled properly and Qt only rebuilds if those files change - which would be better than the script.

@lilyinstarlight
Copy link
Contributor

lilyinstarlight commented Feb 21, 2022

@cmaughan @samaaron I just quickly tried this on my system with Qt5 and it correctly only rebuilt the .qm files when the corresponding .ts files changed. Subsequent runs did not rebuild any files that did not change and if none were changed, it did not bother rebuilding SonicPi.qrc or relinking sonic-pi either. So this seems to work correctly on Qt5 (I would need to fire up a new VM to test Qt6 and on other platforms myself)

Build log
[I] lily@bina ~/s/s/a/build (dev)> make
[  4%] Built target sonic-pi-api
[  5%] Built target api-tests
[  5%] Automatic MOC and UIC for target QScintilla
[  5%] Built target QScintilla_autogen
[ 75%] Built target QScintilla
[ 76%] Automatic MOC and UIC for target sonic-pi
[ 76%] Built target sonic-pi_autogen
[100%] Built target sonic-pi
[I] lily@bina ~/s/s/a/build (dev)> touch ../gui/qt/lang/sonic-pi_en_US.ts
[I] lily@bina ~/s/s/a/build (dev)> make
[  4%] Built target sonic-pi-api
[  5%] Built target api-tests
[  5%] Automatic MOC and UIC for target QScintilla
[  5%] Built target QScintilla_autogen
[ 75%] Built target QScintilla
[ 76%] Automatic MOC and UIC for target sonic-pi
[ 76%] Built target sonic-pi_autogen
[ 77%] Generating ../../../gui/qt/lang/sonic-pi_en_US.qm
Updating '/home/lily/src/sonic-pi/app/gui/qt/lang/sonic-pi_en_US.qm'...
    Generated 309 translation(s) (309 finished and 0 unfinished)
[ 78%] Automatic RCC for SonicPi.qrc
[ 78%] Building CXX object gui/qt/CMakeFiles/sonic-pi.dir/sonic-pi_autogen/EWIEGA46WW/qrc_SonicPi.cpp.o
[ 78%] Linking CXX executable sonic-pi
[100%] Built target sonic-pi
[I] lily@bina ~/s/s/a/build (dev)> make
[  4%] Built target sonic-pi-api
[  5%] Built target api-tests
[  5%] Automatic MOC and UIC for target QScintilla
[  5%] Built target QScintilla_autogen
[ 75%] Built target QScintilla
[ 76%] Automatic MOC and UIC for target sonic-pi
[ 76%] Built target sonic-pi_autogen
Consolidate compiler generated dependencies of target sonic-pi
[100%] Built target sonic-pi
[I] lily@bina ~/s/s/a/build (dev)>

@SunderB
Copy link
Contributor Author

SunderB commented Feb 21, 2022

Rebasing has been done :)

Copy link
Contributor

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

Can you remove the QT5_BIN_DIR-related lines at the top of mac-prebuild.sh too, since those are now unnecessary/unused?

(random side-note, but it seems I can't add comments in the GitHub interface around lines that weren't modified so I linked them above instead)

@SunderB
Copy link
Contributor Author

SunderB commented Feb 28, 2022

Done :)

@samaaron samaaron merged commit cd68cb8 into sonic-pi-net:dev Mar 1, 2022
@samaaron
Copy link
Collaborator

samaaron commented Mar 1, 2022

Thanks for this :-)

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.

4 participants