search

Komet / MediaElch

Media Manager for Kodi
https://mediaelch.github.io/mediaelch-doc/about.html
GNU Lesser General Public License v3.0
835 stars 94 forks source link

`safe_int_cast` breaks builds on i386 #1511

Closed rvandegrift closed 1 year ago

rvandegrift commented 1 year ago

The check at https://github.com/Komet/MediaElch/blob/master/src/utils/Meta.h#L234 breaks builds on i386:

/<<PKGBUILDDIR>>/src/utils/Meta.h: In instantiation of ‘constexpr To safe_int_cast(From) [with To = int; From = int]’:
/<<PKGBUILDDIR>>/src/utils/Meta.h:208:30:   required from here
/<<PKGBUILDDIR>>/src/utils/Meta.h:194:44: error: static assertion failed: Unnecessary int cast: Both types are the same
  194 |     static_assert(!std::is_same<From, To>::value, "Unnecessary int cast: Both types are the same");
      |                                            ^~~~~
/<<PKGBUILDDIR>>/src/utils/Meta.h:194:44: note: ‘!(bool)std::integral_constant<bool, true>::value’ evaluates to false

A full build log is at: https://salsa.debian.org/rvandegrift/mediaelch/-/jobs/3681235/raw

To Reproduce Steps to reproduce the behavior:

  1. build on i386

Expected behavior Casts that become redundant due to machine word size should be okay.

MediaElch Version:

Operating System:

bugwelle commented 1 year ago

Hi,

Oh, I didn't know that people still build MediaElch for 32bit. Otherwise I may have checked it. :)

Thanks for reporting it.

Out of curiosity: what script do you use to package MediaElch? (packaging for Debian was always a lot of guessing for me, though it's gotten better).

(I'm on mobile ; if I get to it, I will fix it between the holidays, but a pull request would also be welcome :))

Officially, I don't support 32bit anymore. I just don't have a system to test it on and many Linux distributions switched to 64bit only.

Regards, Andre

bugwelle commented 1 year ago

I see that you use Qt6 for your Debian build, awesome! May I use https://salsa.debian.org/rvandegrift/mediaelch/-/commit/2c5a249b21f9743f969da794331ba6860fb76937#58ef006ab62b83b4bec5d81fe5b32c3b4c2d1cc2 in MediaElch's main project line as well (at least document it)?

Just some one note:

If there is anything I can do to make your life a little bit easier (such as maintaining a proper copyright file), please let me know. :)

bugwelle commented 1 year ago

@rvandegrift This will be fixed by #1512. Let me know if there are other places that need fixing. I don't have 32-bit Qt installed on my system or on our Jenkins instance, but I will try to check it in the new year. :)

Please also note that 2.8.18 has some major bugs... I will release a new version in January. If you like, you can cherry-pick these commits:

rvandegrift commented 1 year ago

Hi @bugwelle - thanks for the fix, as well as the pointers to other patches. I also don't use 32-bit systems anymore, but i386 is still a release architecture for Debian. So all packages are built for 32-bit systems too.

The software I use to help with packaging:

Finally, of course you can use anything from my packaging that you like - it's under LGPL-3 only to match mediaelch.

bugwelle commented 1 year ago

cme

Awesome, I didn't know that exists! Btw, can the GitLab CI configuration file be found anywhere? I'm interested in the CXXFLAGS, etc. that it uses. :-)

But beyond that, mediaelch wasn't too hard to get packaged, so thanks!

That's great to hear! Thanks. I see you also found DISABLE_UPDATER :+1: I don't have a package-maintainer guide, yet, but I sometimes put entries in our changelog, e.g.

USE_EXTERN_QUAZIP and DISABLE_UPDATER are the important ones. I am thinking about renaming those variables to MEDIAELCH_*, but will ensure backwards compatibility for those two CMake variables.

Finally, of course you can use anything from my packaging that you like - it's under LGPL-3 only to match mediaelch.

Thanks! I know it's LGPL-3, but I think it's nicer to ask as well. :-)

Do you want to be part of our Release-Team? I use that only for notifications about new releases, so there is no obligation to develop anything. :-)

bugwelle commented 1 year ago

I've added a "Package Maintainer Notes" section to https://github.com/Komet/MediaElch/blob/master/docs/contributing/packaging.md#package-maintainer-notes

rvandegrift commented 1 year ago

cme

Awesome, I didn't know that exists! Btw, can the GitLab CI configuration file be found anywhere? I'm interested in the CXXFLAGS, etc. that it uses. :-)

You can find the pipeline in this repo: https://salsa.debian.org/salsa-ci-team/pipeline - it's a centralized project to help test packages for Debian.

CXXFLAGS (and other build configurations) come from the packaging. Build flags are generated by dpkg-buildflags - it's man page has some details, including on the environment variables that customize it's output.

But beyond that, mediaelch wasn't too hard to get packaged, so thanks!

That's great to hear! Thanks. I see you also found DISABLE_UPDATER +1 I don't have a package-maintainer guide, yet, but I sometimes put entries in our changelog, e.g.

* https://github.com/Komet/MediaElch/blob/45ad60ab18a173d84337a3f6b14fc70286d667bb/CHANGELOG.md#changes-for-package-maintainers

USE_EXTERN_QUAZIP and DISABLE_UPDATER are the important ones. I am thinking about renaming those variables to MEDIAELCH_*, but will ensure backwards compatibility for those two CMake variables.

Cool, thanks for the heads up - I found the existing notes helpful, especially when looking at Qt5 vs Qt6. Migrating to new variables for this purpose should be easy.

Do you want to be part of our Release-Team? I use that only for notifications about new releases, so there is no obligation to develop anything. :-)

Sure, thanks!

bugwelle commented 1 year ago

Thank you for your detailed answer! I've invited you to the Release-Team. :)

If MediaElch is packaged for Debian, are we talking about the official Debian repositories? (we only have a PPA for Ubuntu at the moment).

What are the guidelines on releasing patches or minor releases you have to follow? Because currently, MediaElch does not follow semantic versioning and patch-releases sometimes contain new features. Minor releases definitely contain new features. I don't introduce breaking changes on purpose, but sometimes deprecated features such as Kodi-v16 support are removed.

rvandegrift commented 1 year ago

If MediaElch is packaged for Debian, are we talking about the official Debian repositories? (we only have a PPA for Ubuntu at the moment).

Yes - it's my intention to get this uploaded to the Debian archive. It should be ready in time for the next stable release.

What are the guidelines on releasing patches or minor releases you have to follow?

The tl, dr: I don't think there's much of an issue for mediaelch. Details:

In Debian, all new software gets uploaded to the unstable distribution. Normally, I test to ensure that a new upstream release works before uploading it - but it's fine if this includes backwards incompatible changes.

In a stable stable release, software packages normally never change - the point is to ensure that end-users don't have unexpected changes in functionality. It's /possible/ to upload a new version to stable, but it's unlikely to ever be required for mediaelch.

Somewhat more likely would be uploading a security fix. Most of the time: I'd find the fix in your development branch, backport it to the version in stable, and upload the result. I might need to ask you some questions if I got stuck, but you'd probably never need to do any dev work on it.

When upgrading from one stable release to the next, it's expected that some software might gain/lose/change features. That'd be documented in the package changelog or news as appropriate.

Because currently, MediaElch does not follow semantic versioning and patch-releases sometimes contain new features. Minor releases definitely contain new features. I don't introduce breaking changes on purpose, but sometimes deprecated features such as Kodi-v16 support are removed.

Since mediaelch is primarily a desktop application, I don't think this is a problem at all. It'd be good to have it documented in the release notes/changelog so users would know to hold off if they depend on an old version of kodi.

If a shared library introduces breaking changes without appropriate version changes, that can cause a problem. But that's not relevant here.

bugwelle commented 1 year ago

Thank you again for your detailed response. :smile: