-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Support ubuntu jammy, noble, and oracular with their appropriate qt #791
base: main
Are you sure you want to change the base?
Conversation
559fe8b
to
7cd3ee1
Compare
@adamlamar Thank you for the PR. @veloman-yunkan will have a look to this PR this WE. |
kiwixbuild/builder.py
Outdated
for config in ConfigInfo.all_running_configs.values(): | ||
# get {host}_{config} packages | ||
mapper_name = "{host}_{config}".format( | ||
host=neutralEnv("distname"), config=config | ||
) | ||
package_name_mapper = PACKAGE_NAME_MAPPERS.get(mapper_name, {}) | ||
packages_list += package_name_mapper.get("COMMON", []) | ||
# get {host}_{codename}_{config} packages | ||
mapper_name = "{host}_{codename}_{config}".format( | ||
host=neutralEnv("distname"), codename=neutralEnv("codename"), config=config | ||
) | ||
package_name_mapper = PACKAGE_NAME_MAPPERS.get(mapper_name, {}) | ||
packages_list += package_name_mapper.get("COMMON", []) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the code duplication introduced here and in the other change in this (_get_packages()
) function.
You can avoid it with the help of an auxiliary function _get_mapper_names_for_config(config)
:
def _get_mapper_names_for_config(config):
host = neutralEnv("distname")
codename = neutralEnv("codename")
return ( f"{host}_{config}", f"{host}_{codename}_{config}" )
Then your code here will turn into:
for config in ConfigInfo.all_running_configs.values():
for mapper_name in _get_mapper_names_for_config(config):
package_name_mapper = PACKAGE_NAME_MAPPERS.get(mapper_name, {})
packages_list += package_name_mapper.get("COMMON", [])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood about the duplication. There aren't any unit tests so I was avoiding any refactors, but I can adjust as you requested. May need a few days before I can get to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cleaned up nicely. Let me know what you think.
@adamlamar I don't really understand the whole logic behind this PR... but it seems to assume that jammy will always build in Qt5 which is wrong: our CD should statically compile Kiwix Desktop on jammy (because we don't want to compile against a recent glibc) and this should be with Qt6. It seems we have here some kind of double constraints: having at least one CI with Qt5 (to keep compatibility checking) and Jammy is the most appropriate... but our CD should use Jammy to build release linux binaries with Qt6. To me it seems we need to have Qt5 AND Qt6 installed on the Jammy base container... and then choose the version of Qt based on the need (CI or CD). |
@kelson42 This PR only provides more flexibility in the package names used for a specific distribution name and codename. This is important because 1) debian and ubuntu don't always have the same packages (I believe Without this, kiwix-build is broken on all ubuntus (even jammy IIRC) because it doesn't supply the right package list. I think jammy does work if you install the right packages ahead of invoking kiwix-build, but not on a fresh install. I could be misremembering, I jumped around to a lot of different VMs when I was testing. This PR fixes kiwix-build on the recent ubuntus. Although kiwix-desktop itself tests on jammy, noble, and oracular, kiwix-build was not working properly. And while kiwix-build tests many architectures, AFAICT there are currently no CI/CD jobs that test kiwix-build specifically on the ubuntus except for a few combinations. We could definitely install qt5 and qt6 packages for jammy and then choose which one to use at build time. I'll work on that in a couple of days. |
For the record,
|
Thank you for the clarification, sounds obvious to me now :)
Thank you, even if this is something I could do. Should pretty trivial. Might do straight a PR. Edit: See kiwix/container-images#273 |
@adamlamar Out of curiosity, have you run kiwix-build on Fedora (34?)? I ask because of #504, maybe we have fixed this issue as well already? |
@kelson42 I updated I haven't tested on Fedora. Since Fedora 34 is EOL maybe we should target Fedora 40 or 41? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second commit combines two unrelated changes. The ideal structure for this PR is as follows:
- Refactor the old code by introducing the
_get_mapper_names_for_config()
(that returns a single-element(f"{host}_{config}",)
tuple) - Add support for ubuntu jammy, noble, and oracular with their appropriate qt (enhance the
_get_mapper_names_for_config()
method here) - Use qt6 by default on jammy
Adds package profiles for various versions of ubuntu - jammy, noble, oracular, and uses their appropriate versions of Qt.
When specifying packages, you can now use the LSB
codename
in thePACKAGE_NAME_MAPPERS
dictionary, giving more control about whether (say) a qt package should be installed as version 5 or 6. For exampleubuntu_jammy_native_dyn
andubuntu_noble_native_dyn
are now checked to find the package list. Similarlydebian_bookworm_native_dyn
could be used, but I didn't change any existing package lists.