[CM P0] implement external iterators

[markus2330]

As written in #2991 we want to switch to external iterators. Which bindings do still make use of the internal iterators (ksNext and friends?). They should be removed/deprecated and an external iterator provided instead.

• [x] #4281
• [x] Java (remove internal)
• [X] Rust
• [X] Go
• [X] GLIB
• [ ] #4280
• [x] #4279

---

The functions that will be removed are:

int ksRewind (KeySet *ks);
Key *ksNext (KeySet *ks);
Key *ksCurrent (const KeySet *ks);

elektraCursor ksGetCursor (const KeySet *ks);
int ksSetCursor (KeySet *ks, elektraCursor cursor);

int keyRewindMeta (Key *key);
const Key *keyNextMeta (Key *key);
const Key *keyCurrentMeta (const Key *key);

[manuelm]

Both python and lua are using the CPP NameIterator and KeySetIterator. So if you remove them you are also removing them from python+lua.

[ghost]

ksNext and friends?

What is "friends" in here?

The java implementation already uses an external iterator

Once ksNext() is removed, this line should also be removed.

[manuelm]

@Piankero I assume ksRewind, ksNext, ksCurrent, ksGetCursor, ksSetCursor

@markus2330 You should add Ruby. It's actually wrapping all five and also using them in the ruby iterator implementation

[markus2330]

Thank you for the hint, I added Ruby (@BernhardDenner).

@Piankero can you fix Java, it should be quite easy for you to write an iterator?

[ghost]

@Piankero can you fix Java, it should be quite easy for you to write an iterator?

There is already an external iterator written as I have written in the upper comment.

[markus2330]

Even better, then you only need to remove the internal iterator, I updated the description above.

[ghost]

@manuelm @markus2330 Are you sure about the ksRewind? If I remove this line I receive test failures in the integration tests of jna.

[markus2330]

Yes, the tests also need update.

[ghost]

Yes, the tests also need update.

I already have updated some test cases but the set/get methods seem to need a rewinded keyset. what should I do if I cannot rewind it anymore?

[markus2330]

Thank you this is really a good point. I am afraid we need to fix all plugins that use the internal cursor to properly rewind before using the iterator.

We also need some docu (in the release notes) how to use the external iterator and how to migrate away from the internal iterator:

1. we only use external iterator from now on.
2. after 1.0 release we slowly start replacing usage of internal iterator (to use external iterator instead)

[markus2330]

ksGet/SetCursor is also a friend of ksNext, I opened #3189 for that.

[ghost]

This issue is unrelated to visible error messages. As discussed in the Elektra meeting I may unassign myself.

[markus2330]

@Piankero this would be a quite simple Java task.

[ghost]

Yes it is an easy task but I cannot remove the deprecated iterator because the tests use plugins that expect a rewinded keyset and I cannot rewind them any longer. Until those plugins are fixed I could not do that anyhow.

Nontheless we discussed that only error message specific tasks are prioritized because of time constaints so this will have to be done by someone else unfortunately. :(

[kodebach]

At least some of the internal iterator functions exposed by the C++ API are not use at all (or only by test cases) and could therefore be removed right now without breaking anything (in our repo).

[markus2330]

The iterators need more thought, I do not think it is suitable for CM.

Only @tucek could look into it for Java. (Removing ksNext, ksCurrent and so on.)

[tucek]

So what functions are you planning to remove exactly?

• [ ] ksRewind
• [ ] ksNext
• [ ] ksCurrent
• [ ] ksGetCursor
• [ ] ksSetCursor
• [ ] ksAtCursor
• [ ] anything else?

[kodebach]

I think you've got most of it. But ksAtCursor needs to stay and just be renamed to ksAt. It doesn't use the internal cursor, but takes an external cursor value. Also elektraKsPopAtCursor should replace the current ksPop. See also #3189.

IMO, the end goal would be to remove Key * cursor and size_t current from struct _KeySet. So anything that uses it and doesn't serve another purpose (e.g. ksLookup sets the cursor, but clearly has a different purpose) should be removed. Anything that uses it, but has another purpose needs to be modified.

[tucek]

@markus2330 @kodebach

• i currently cannot rename Elektra::ksAtCursor to Elektra::ksAt, since it is not yet renamed in the native library. Regarding the JNA binding, it does not really matter anyways, because it already features the method with signature Key KeySet::at(Pointer, int)...
• i've removed Elektra::ksNext, Elektra::ksCurrent, Elektra::ksGetCursor, Elektra::ksSetCursor, KeySet::next, KeySet::current, KeySet::rewind, KeySet::getCursor, KeySet::setCursor
• i've deprecated Elektra::ksRewind, since it is needed to rewind the a KeySet before handing it to a native plugin via NativePlugin::set, NativePlugin::get and NativePlugin::error

btw. i'm getting the following output for some test in master (test are not failing):

Running org.libelektra.GOptsTest
Invalid name:  (in keyVNew at src/libs/elektra/key.c:238)
Invalid name:  (in keyVNew at src/libs/elektra/key.c:238)
Invalid name:  (in keyVNew at src/libs/elektra/key.c:238)
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.038 sec

Running org.libelektra.KDBTest
Invalid name:  (in keyVNew at src/libs/elektra/key.c:238)
Invalid name:  (in keyVNew at src/libs/elektra/key.c:238)
Invalid name:  (in keyVNew at src/libs/elektra/key.c:238)
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.001 sec

I've also noticed that KeySetIterator is yet missing tests.

see draft PR #3706

[kodebach]

• i've deprecated Elektra::ksRewind, since it is needed to rewind the a KeySet before handing it to a native plugin via NativePlugin::set, NativePlugin::get and NativePlugin::error

I haven't looked that closely at the Java Binding, but could you call Elektra::ksRewind inside the NativePlugin methods and make it package-private to essentially remove it? Or is that not possible?

btw. i'm getting the following output for some test in master (test are not failing): [...]

The "Invalid name: " messages are log messages, these will not cause test errors automatically. However, if you are not explicitly testing for invalid key names, then there shouldn't be any such messages.

In this case something tries to call keyNew ("", KEY_END) (possible with more arguments before KEY_END). You can tell that this happens, since the full log message should be Invalid name: <KEYNAME> (in keyVNew at src/libs/elektra/key.c:238). So in this case <KEYNAME> must be an empty string.

Normally, to debug this, I would suggest adding a breakpoint at src/libs/elektra/key.c:238 and looking at the call stack, to find the problem. But I don't think adding breakpoints to the native code is easily possible with JNA, so you'll have to manually check where the invalid key name comes from. If it only happens in the GOptsTest it shouldn't be hard to find, just check the parts of the binding that are only used there.

[tucek]

• i've deprecated Elektra::ksRewind, since it is needed to rewind the a KeySet before handing it to a native plugin via NativePlugin::set, NativePlugin::get and NativePlugin::error

I haven't looked that closely at the Java Binding, but could you call Elektra::ksRewind inside the NativePlugin methods and make it package-private to essentially remove it? Or is that not possible?

I could only make the Elektra interface as a whole package private. This would also require the NativePlugin class to be moved to the same package as the Elektra interface. In my opinion this would not make things better. Explicitly annotating the Elektra::ksRewind as 'deprecated for removal' should suffice imho.

btw. i'm getting the following output for some test in master (test are not failing): [...]

The "Invalid name: " messages are log messages, these will not cause test errors automatically. However, if you are not explicitly testing for invalid key names, then there shouldn't be any such messages.

In this case something tries to call keyNew ("", KEY_END) (possible with more arguments before KEY_END). You can tell that this happens, since the full log message should be Invalid name: <KEYNAME> (in keyVNew at src/libs/elektra/key.c:238). So in this case <KEYNAME> must be an empty string.

Normally, to debug this, I would suggest adding a breakpoint at src/libs/elektra/key.c:238 and looking at the call stack, to find the problem. But I don't think adding breakpoints to the native code is easily possible with JNA, so you'll have to manually check where the invalid key name comes from. If it only happens in the GOptsTest it shouldn't be hard to find, just check the parts of the binding that are only used there.

I've debugged the affected Java tests. the only thing i can see so far is, that it happens after kdb.set operations GOptsTest.java:55 and KDBTest.java:44.

Anyways i cannot see an adverse effect regarding the tests and since this was already an issue in master i would deem it out of scope of #3171.

[kodebach]

Anyways i cannot see an adverse effect regarding the tests and since this was already an issue in master i would deem it out of scope of #3171.

If you don't fix it, please create a separate issue, so we don't forget to fix it later

[tucek]

Anyways i cannot see an adverse effect regarding the tests and since this was already an issue in master i would deem it out of scope of #3171.

If you don't fix it, please create a separate issue, so we don't forget to fix it later

I've created #3710 to address this issue.

[tucek]

@kodebach Was the Java binding the last to be updated and we can now close this issue?

[kodebach]

I'm not sure, let's keep the issue open for now. But your part is done, so I unassigned you...

[markus2330]

@kodebach can you write a new issue about rewriting the parts that still use ksNext/ksRewind etc.? In particular rewriting the plugins would be quite suitable for CM.

[kodebach]

@kodebach can you write a new issue about rewriting the parts that still use ksNext/ksRewind etc.? In particular rewriting the plugins would be quite suitable for CM.

I created #4279, #4280, #4281. Please label them with the appropriate points for CM.

[github-actions[bot]]

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

[github-actions[bot]]

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart: