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

Consider setting SONAME to major.minor #2002

Closed

Conversation

Undef-a
Copy link

@Undef-a Undef-a commented Jun 30, 2022

This allows Calamares extensions built against a patch version to be run against another patch version. It fixes the mobile (and similar) modules silently breaking when Calamares is independently updated.

Specifically, this should help mitigate issues like https://gitlab.com/mobian1/issues/-/issues/438, where an update from Debian's Calamares breaks all usage of Calamares extensions. Right now we see this issue crop up most months as Calamares and Calamares Extensions are maintained by separate people.

The commit in this PR implements this, but I'm opening the PR more as a discussion on whether we could make this change in libcalamares than as a preferred way of doing it.

Thanks for considering.

This allows calamares extensions built against a patch version to be run
against another patch version. It fixes the mobile (and similar) modules
silently breaking when calamares is independently updated.
@spaetz
Copy link

spaetz commented Jun 30, 2022

If you don't want two places in which you need to bump version numbers, you could even construct the SO Version number from CALAMARES_VERSION_MAJOR CALAMARES_VERSION_MINOR which we have available, so it would work automagically?!

@@ -44,6 +44,7 @@
cmake_minimum_required(VERSION 3.16 FATAL_ERROR)

set(CALAMARES_VERSION 3.3.0-alpha1)
set(CALAMARES_SOVERSION 3.3)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you move this below the project() command you can use the major, minor parts and don't have to repeat it here. You could even skip setting this entirely and just use major.minor down below, but you could also just choose to set this to "4" and call that the soversion.

Copy link
Contributor

Choose a reason for hiding this comment

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

There has never been a promise of ABI stability in Calamares; the functions in libcalamares have changed a lot over the course of the 3.2 series, always with the Calamares version as SO version: there's no guarantee any two of the 3.2.x versions were in any way compatible with each other. Once 3.3 is actually released, then we might consider ABI stability (in which case 3.3, or 4, might make sense as an SO version).

If we use the ci/abicheck.sh script we could keep track of actual compatibility -- otherwise the major.minor SO version is just a lie. You can make use of that script: for instance, run it and add the output to ci/ as, I dunno, abi-so-3.3. Then, in the release script, run the abi check and compare against the stored text. Then we can make sure that the SO version is useful and correct.

@kkofler
Copy link
Contributor

kkofler commented Jun 30, 2022

A pull request is not a good place to discuss ABI stability policies. The ABI policies need to be discussed and agreed on before it makes sense to submit an implementation.

@kkofler kkofler closed this Jun 30, 2022
@adriaandegroot adriaandegroot reopened this Jul 2, 2022
@adriaandegroot
Copy link
Contributor

(The contribution guide suggests exactly what is happening here: open a PR early, in order to discuss whether this is a worthwhile avenue to pursue; unless there are qualitatively-different discussion possibilities elsewhere, this is the best place)

So to reiterate:

  • having a 3.3 SO (or whatever) might be useful, but only if you also help out in supporting ABI stability,
  • for the 3.2 series, we know that having a 3.2 SO version would not have worked: instead of "missing 3.2.x" library, modules would link to 3.2 and then mysteriously crash because the ABI changed out from under them.

A PR that addresses only the "make a symlink" part is insufficient, but can be expanded with documentation, script integration, etc. (also, remember there's no 3.3.0 yet, so the libraries may change at any time until then).

@adriaandegroot
Copy link
Contributor

@Undef-a closing this again. It needs more work than just setting an SO version, as explained above (effectively, if there's an SO version set the way you propose, then the only realistic thing to do with Calamares as it is developed now, is bump the version every release since nothing else checks for stability -- and let's not forget that along the Arch side, rolling Python or Boost updates break Calamares all the time as well; that's not relevant for your Mobian, but an additional data point)

@Undef-a
Copy link
Author

Undef-a commented Jul 10, 2022

Sorry for the delay, this just made it back to the top of my list today.

To make sure I'm on the right track, this MR would be considered only with a compatibility check in CI for libcalamares and only for 3.3/4.0 when released? That still sounds like a pretty big win to me and I'm happy to work on it.

As an alternative, would it be worth considering just pulling the regularly used calamares-extensions modules into this repository? At least from my perspective, that would remove any need for ABI compatibility as they would all be re-built together.

@adriaandegroot
Copy link
Contributor

Come to think of it:

  • 3.2 should be stable now; it's in long(-ish) term support, so there should be some pressure to keep the ABI stable (if only because no changes are expected)
  • 3.3 should try to be stable in the longer term, or at least be more explicit about ABI changes for the modules (though, like I've said, the major drivers for ABI issues are Boost, Python, and to a far lesser degree, kpmcore)
  • ci/abicheck.sh can already do most of the checks, using libabigail. I think it already keeps around the ABI information after generation, so a straightforward adjustment of the BASE_VERSION (in whichever branch) would work. I think adding the ABI information to the ci/ directory, if it isn't too large, would be ok (just to avoid having to rebuild it every single time)
  • CI could check and report on ABI issues (for instance, in the calamares branch, on push)
  • The release script should check compatibility (in the 3.3 branch, at least). Doesn't have to block a release, but it would be a thing to report in the release notes ("ABI has changed; all C++-based modules must be rebuilt")

As for the calamares-extensions repository, the whole point is that there are non-core modules there (and we could easily argue that several modules in the calamares repository belong over there as well). If there's modules there that are core (and that means "applicable to multiple families of distro's", not "important for your one distro") then that's a separate conversation to be had (it may make sense in the 3.3 pre-release state to go over the modules again).

@adriaandegroot
Copy link
Contributor

Abigail produces this kind of ABI information (for the calamares branch, where we know we're changing the ABI until it looks right for a 3.3.0 release):

  [C] 'method QPair<unsigned long long, float> CalamaresUtils::System::getTotalMemoryB()' at CalamaresUtilsSystem.cpp:234:1 has some sub-type changes:
    return type changed:
      type name changed from 'QPair<unsigned long long, float>' to 'QPair<unsigned long long, double>'
      type size hasn't changed

The abigail information is ~18MB so that's a no-go for adding it directly to the repository. That makes it possibly something to check regularly in CI by re-generating the information, or if CI supports caching artifacts, doing it there.

Undef-a pushed a commit to Undef-a/actions that referenced this pull request Jul 20, 2022
This is a pre-requisite for one of the ABI checks discussed in
calamares/calamares#2002 to eventually be run
against the 3.3/calamares branches.
adriaandegroot added a commit that referenced this pull request Aug 23, 2022
This is basically PR #2002, from Undef-a.
pvshvp-oss pushed a commit to RebornOS-Team/calamares-core that referenced this pull request Dec 1, 2022
This is basically PR calamares#2002, from Undef-a.
pvshvp-oss pushed a commit to RebornOS-Team/calamares-core that referenced this pull request Dec 1, 2022
commit f75b599
Merge: 4ced827 b00a2ed
Author: Adriaan de Groot <groot@kde.org>
Date:   Thu Sep 22 16:29:49 2022 +0200

    Merge pull request calamares#2049 from killajoe/patch-1

    Update services-systemd.conf

commit b00a2ed
Author: Johannes Kamprad <archlinux@kamprad.net>
Date:   Sun Sep 18 14:43:15 2022 +0200

    Update services-systemd.conf

    removing mandatory: false from example for cups-socket as it is default same as for the other two (could confuse about may disable needs to set it? )

commit 4ced827
Author: Adriaan de Groot <groot@kde.org>
Date:   Fri Sep 9 22:10:07 2022 +0200

    Changes: document new bits for 3.3.0-a3

commit 625693a
Author: Adriaan de Groot <groot@kde.org>
Date:   Fri Sep 9 22:09:56 2022 +0200

    CMake: bump dependency versions

commit 1821eb1
Author: Adriaan de Groot <groot@kde.org>
Date:   Sun Aug 7 15:14:13 2022 +0200

    [libcalamaresui] Branding uses $-substitution

    Replace @{name} with ${name} to be consistent with the rest
    of the replacement-code in Calamares.

commit d1664f3
Author: Adriaan de Groot <groot@kde.org>
Date:   Fri Sep 9 09:56:56 2022 +0200

    [services-systemd] Correct Python key-checking

    `has_key()` is a Python2-era form; use the `in` operator insteda.

commit 6235e04
Merge: f9f4c6f 9191748
Author: Adriaan de Groot <groot@kde.org>
Date:   Thu Sep 8 23:14:15 2022 +0200

    Merge pull request calamares#2046 from calamares/dracut_conf

    [dracut] add option to make the kernel name configurable

commit 9191748
Author: demmm <anke62@gmail.com>
Date:   Sat Sep 3 16:22:00 2022 +0200

    Changes: updates for 3.3.0-alpha3

commit 26166e8
Author: demmm <anke62@gmail.com>
Date:   Sat Sep 3 14:36:45 2022 +0200

    [dracut] add asked for schema.yaml
    corrected if statement

commit f9f4c6f
Merge: 7e73797 abdfeaa
Author: Adriaan de Groot <groot@kde.org>
Date:   Sat Sep 3 01:34:49 2022 +0200

    Merge pull request calamares#2045 from demmm/calamares

    [localeq] move to using Drawer for fine tuning options

commit f16da0f
Author: demmm <anke62@gmail.com>
Date:   Fri Sep 2 21:53:42 2022 +0200

    [dracut] add option to make the kernel name configurable

commit abdfeaa
Author: demmm <anke62@gmail.com>
Date:   Fri Aug 26 15:43:47 2022 +0200

    [localeq] move to using Drawer for fine tuning options
    i18n.qml no longer needed
    add color setting options to localeq.qml
    Offline.qml updated to be inline with keyboardq UI, set index number according to default
    America/New York

commit 7e73797
Author: demmm <anke62@gmail.com>
Date:   Thu Aug 25 12:05:07 2022 +0200

    [keyboardq] add missing image license

commit a741663
Author: Adriaan de Groot <groot@kde.org>
Date:   Thu Aug 25 00:26:31 2022 +0200

    SPDX: remove license information for removed module-translations

commit c0672d1
Author: Adriaan de Groot <groot@kde.org>
Date:   Thu Aug 25 00:25:45 2022 +0200

    SPDX: license information for the locales-test-data

commit 842b092
Merge: 90d159a 7876cdc
Author: Adriaan de Groot <groot@kde.org>
Date:   Thu Aug 25 00:19:42 2022 +0200

    Merge pull request calamares#2044 from demmm/calamares

    [keyboardq] Move to using a Drawer

commit 90d159a
Author: Adriaan de Groot <groot@kde.org>
Date:   Wed Aug 24 23:39:24 2022 +0200

    Changes: post-release housekeeping

commit 130d26c
Author: Adriaan de Groot <groot@kde.org>
Date:   Wed Aug 24 23:35:19 2022 +0200

    Changes: close off the 3.2 changelog in the development branch

commit bef731b
Author: Adriaan de Groot <groot@kde.org>
Date:   Wed Aug 24 23:31:36 2022 +0200

    Changes: post-release housekeeping

    Note that in 3.2 branch, the version in CMakeLists now changes just before
    the next release, not in post-release housekeeping. That is because
    the CALAMARES_VERSION_RC remains 0 (release mode), by convention.

    (cherry picked from commit aa09664)

commit b0bd624
Author: Adriaan de Groot <groot@kde.org>
Date:   Wed Aug 24 21:31:53 2022 +0200

    Changes: pre-release housekeeping

    (cherry picked from commit c8eec51)

commit 54e19af
Author: Adriaan de Groot <groot@kde.org>
Date:   Sun Aug 7 14:56:19 2022 +0200

    Changes: document 3.2.61 work so far

    (cherry picked from commit f212119)

commit 0e9cf86
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 23:43:25 2022 +0200

    CI: fix getting-the-version in release script

commit 7876cdc
Author: demmm <anke62@gmail.com>
Date:   Tue Aug 23 18:56:49 2022 +0200

    [keyboardq] Move to using a Drawer
    no longer use a ComboBox or stack view
    if accepted, other QML models using a ComboBox will move to Drawer too

commit 0656471
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 16:04:24 2022 +0200

    [displaymanager] Skip greetd test if there's no toml

    toml is needed for greetd, but that shouldn't stop the tests
    from running.

commit 6558cd5
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 15:45:20 2022 +0200

    CMake: add .so-version

    This is basically PR calamares#2002, from Undef-a.

commit c939fbc
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 15:17:09 2022 +0200

    Changes: pre-release housekeeping

commit c34ee74
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 14:08:50 2022 +0200

    CI: update base for ABI compatibility

commit 66cc1a7
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 12:23:13 2022 +0200

    CMake: apply gersemi to libcalamares

    Since this CMakeLists.txt writes out a C program, the formatting
    is a bit weird; just start the written TU with a blank line to
    make gersemi happy and keep the C-code aligned.

commit 8de565f
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 12:22:09 2022 +0200

    CMake: apply gersemi formatting

commit c3f366c
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 11:48:14 2022 +0200

    CI: check translations against detached branches

    To avoid git complaining about duplicate worktrees, detach
    the temporary trees. To avoid python modules translations
    changing order (depending on how find traverses the tree),
    sort the filenames before extraction.

commit 7a26236
Merge: 115f493 9a4d992
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 02:07:33 2022 +0200

    Merge branch 'issue-2008a' into calamares

    This fixes all the **tests** of locale-detection. Now we can
    test for user interaction.

commit 9a4d992
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 02:06:06 2022 +0200

    [locale] Repair tests

    - Esperanto now doesn't quite self-match because it has no country
    - sr prefers RS as country over ME

commit fb3112b
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 02:02:54 2022 +0200

    [locale] Repair tests

    - prefers language default country (ca_ES over ca_AD)
    - prefers non-empty country match

commit 3540121
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 02:02:24 2022 +0200

    [locale] Prefer non-empty country matches

    Prefer "en_US" over "en" even when asking for "en".

commit a422fd8
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 00:46:40 2022 +0200

    [locale] Refactor matching some more

    - find the best score and match relative to a specific
      set of parts; make it easy to update the country-setting
    - look for a complete match, or best match, with three
      country settings

commit 6cbf2d7
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Aug 23 00:03:04 2022 +0200

    [locale] Factor out the guess-language part

commit 40527ff
Author: Adriaan de Groot <groot@kde.org>
Date:   Mon Aug 22 23:48:21 2022 +0200

    [locale] Be more chatty while matching locales

commit eb24216
Author: Adriaan de Groot <groot@kde.org>
Date:   Sun Aug 14 21:45:45 2022 +0200

    [locale] Log what we matched with (for language)

commit cfb8ef9
Author: Adriaan de Groot <groot@kde.org>
Date:   Sun Aug 14 17:16:31 2022 +0200

    [locale] Use locale-similarity for searching

commit a988298
Author: Adriaan de Groot <groot@kde.org>
Date:   Sun Aug 14 17:16:12 2022 +0200

    [localeq] Needs more shared sources from locale

commit 78e216f
Author: Adriaan de Groot <groot@kde.org>
Date:   Sun Aug 14 16:26:46 2022 +0200

    [locale] Introduce a similarity-score for locales

commit 115f493
Author: Adriaan de Groot <groot@kde.org>
Date:   Sun Aug 7 14:43:06 2022 +0200

    [bootloader] Repair Python 3.6 compatibility

    Argument *text* is an addition in 3.7, while the Calamares 3.3
    branch supports Python 3.6 and later. Use the 'backwards compatibility'
    name of the parameter, *universal_newlines*.

    Cherry-picked from 33961ff (in the 3.2 branch, though, Python 3.3
    is supported).

commit fd56b5b
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Jul 26 22:10:46 2022 +0200

    [locale] Approach matching from a different angle

    - add struct that splits a locale name into parts
    - add tests that the splitting and joining works

commit 84c0da2
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Jul 19 18:56:50 2022 +0200

    [locale] Test KDE neon and FreeBSD separately, same data

    - wrangle the test framework so it hands the same data to
      two different collections of tests; do KDE neon and FreeBSD
      separately so it's clearer which lookups are being done
      (and a failure in one doesn't prevent the test of the other).

commit d52d1bf
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Jul 19 18:48:32 2022 +0200

    [locale] Add FreeBSD test data for locale-mapping

commit 73628b1
Author: Adriaan de Groot <groot@kde.org>
Date:   Tue Jul 19 18:05:24 2022 +0200

    [locale] Add test for language-mapping

    Adds specific data from KDE neon and expected mappings.
    The test fails right now because the mapping is incorrect.
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

4 participants