Tests fail with parted 3.5 because msdos and gpt disk types support new features

[AdamWill]

pyparted tests fail with parted 3.5. The DiskTypeStrTestCase tests in tests/test__ped_disktype.py expect the msdos type to have 1 'feature', and the 'gpt' type to have 2 'features'. But with parted 3.5, they report 5 and 10 'features' respectively:

>>> ty = _ped.disk_type_get_next(ty)
>>> print(ty)
_ped.DiskType instance --
  name: gpt  features: 10
...
>>> ty = _ped.disk_type_get_next(ty)
>>> print(ty)
_ped.DiskType instance --
  name: msdos  features: 5

downgrading to parted 3.4, without changing anything else, results in the expected output. I don't know what it is about parted 3.5 that makes these types appear to have so many more features, yet.

[AdamWill]

Ah, I think I see it. This is kinda expected: upstream commit https://git.savannah.gnu.org/cgit/parted.git/commit/?id=61b3a9733c0e0a79ccc43096642d378c8706add6 adds a couple of new features , PED_DISK_TYPE_PARTITION_TYPE_ID (constant 4) and PED_DISK_TYPE_PARTITION_TYPE_UUID (constant 8). The number we're checking here is a bitmask (in decimal), not a count. The msdos type supports PED_DISK_TYPE_PARTITION_TYPE_ID and the gpt type supports PED_DISK_TYPE_PARTITION_TYPE_UUID , so these values are correct - we add 4 to msdos (so the bitmask goes from 1 to 5) and 8 to gpt (so the bitmask goes from 2 to 10).

So we just need to adjust the tests in pyparted, I guess, to expect different values depending on the parted version.

[hroncok]

parted.version()['libparted'] gives the string representation of libparted version.

[AdamWill]

Well, this turned into quite a yak shave!

@hroncok we shouldn't really use that here, because we're just using _ped. But that's okay, because you can get the libparted version from _ped too. However, I don't really want to do that. Parsing version numbers sucks, especially in code that still cares about Python < 3.6.

So I poked about and turns out there are these nice _FIRST_ and _LAST_ macros in parted that are tailor made for this sort of thing. And there's even existing code in pyparted that uses one of them. So, easy, right? We just copy that approach for these new features:

#if PED_DISK_TYPE_LAST_FEATURE > 2
    PyModule_AddIntConstant(m, "DISK_TYPE_PARTITION_TYPE_ID", PED_DISK_TYPE_PARTITION_TYPE_ID);
    PyModule_AddIntConstant(m, "DISK_TYPE_PARTITION_TYPE_UUID", PED_DISK_TYPE_PARTITION_TYPE_UUID);
#endif

great! So, uh, I did that. And it doesn't work. The constants don't get added when building against parted 3.5. I got very frustrated trying to work out why the hell it didn't work when the existing code did, till I finally realized, uh, the existing code doesn't work:

>>> hasattr(_ped, 'PARTITION_MSFT_DATA')
False

that's using Rawhide's pyparted, which certainly should be built against a parted with the PED_PARTITION_MSFT_DATA flag since that's pretty old. So I looked around a bit, and good old Stackoverflow tells us this can't possibly work. The preprocessor doesn't handle enums, so all the _FIRST_ and _LAST_ macros are gonna evaluate to 0 at preprocessor time.

I noticed that @bcl had worked this out in 2020 and tried to fix it by doing the checks at compile/run time instead, in pyparted 3813e9e9723d43fea7ffd7ec4477d9e774e09600 . @dcantrell changed it back to using the preprocessor only recently, in 54fbbd938fc5b3420c7dde35681d5e75082554a4 , without explaining why or apparently engaging at all with the 'preprocessor doesn't know about enums' problem. So, okay, let's just revert it to the old way and things will be fine, right? So we do this instead:

if (PED_DISK_TYPE_LAST_FEATURE > 2) {
    PyModule_AddIntConstant(m, "DISK_TYPE_PARTITION_TYPE_ID", PED_DISK_TYPE_PARTITION_TYPE_ID);
    PyModule_AddIntConstant(m, "DISK_TYPE_PARTITION_TYPE_UUID", PED_DISK_TYPE_PARTITION_TYPE_UUID);
}

...only, uh, turns out that doesn't work either. It builds fine and does what we want against parted 3.5, but if you try and build it against parted 3.4, it blows up, because just being in this if block doesn't stop the compiler wanting those identifiers to be declared:

src/_pedmodule.c: In function ‘PyInit__ped’:
src/_pedmodule.c:689:63: error: ‘PED_DISK_TYPE_PARTITION_TYPE_ID’ undeclared (first use in this function); did you mean ‘PED_DISK_TYPE_PARTITION_NAME’?
  689 |     PyModule_AddIntConstant(m, "DISK_TYPE_PARTITION_TYPE_ID", PED_DISK_TYPE_PARTITION_TYPE_ID);
      |                                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                               PED_DISK_TYPE_PARTITION_NAME
src/_pedmodule.c:689:63: note: each undeclared identifier is reported only once for each function it appears in
src/_pedmodule.c:690:65: error: ‘PED_DISK_TYPE_PARTITION_TYPE_UUID’ undeclared (first use in this function); did you mean ‘PED_DISK_TYPE_PARTITION_NAME’?
  690 |     PyModule_AddIntConstant(m, "DISK_TYPE_PARTITION_TYPE_UUID", PED_DISK_TYPE_PARTITION_TYPE_UUID);
      |                                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                                 PED_DISK_TYPE_PARTITION_NAME

so basically, I'm just an idiot monkey when it comes to C, but I'm damned if I can see any way you can possibly use these macros as they're currently done. I've sent a mail to upstream parted-devel list outlining the problem and proposing to just define the _FIRST_ and _LAST_ macro values directly, which means they're duplicated with the enum entries but should at least work properly and let you use them at preprocessor time.

I'm either gonna do a 'bad' patch for pyparted in Rawhide which would not be upstreamable because it wouldn't build against 3.4 but will be fine for Rawhide, or I'll apply my parted patch downstream in Rawhide and do a 'proper' pyparted patch.

[hroncok]

As an ugly hack we could change the assert to check the number is either of 1 or 5 (and 2 or 10).

[AdamWill]

That's completely wrong for upstream (i.e. here), it ignores the rather large underlying problem. For downstream it'd be okay (and was my initial plan) but I think fixing it more properly is better, and that's what I've done (I did a parted build with the macros defined directly, and a pyparted build against that with a proper patch).

[hroncok]

To clarify, I was suggesting that for downstream only.

[dcantrell]

Resolved in pyparted-3.13.0