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

Construct: the order of Computed fields matter #1106

Open
Mingun opened this issue Apr 15, 2024 · 4 comments
Open

Construct: the order of Computed fields matter #1106

Mingun opened this issue Apr 15, 2024 · 4 comments

Comments

@Mingun
Copy link

Mingun commented Apr 15, 2024

Currently some tests on https://ci.kaitai.io/ are failed because of that fact that instance fields are sorted alphabetically (due to SortedMap used for storage) and generated in the same order.

Sorting of instances are good thing which will ensure stable output friendly for diffs, but Construct currently cannot calculate dependents inside of Computed fields and raises an error instead.

Although problem seems to be resolved on Construct side, it also could be useful to generate Computed properties in a topological order of dependencies.

@generalmimon
Copy link
Member

it also could be useful to generate Computed properties in a topological order of dependencies.

No, the real cause of the problem is that Kaitai Struct instances are supposed to be lazy (see https://doc.kaitai.io/user_guide.html#_instances_data_beyond_the_sequence), but they are eager in the Construct target because KSC implements them using Computed, which is an eager construct.

This problem has been already discussed in #377 - apparently, @arekbulski added Lazy to Construct more or less to support translating .ksy files to Construct, as far as I understand. In #377 (comment), @GreyCat tried to use it, but it wasn't implemented yet.

Nowadays, I hope it has been implemented since then, because https://construct.readthedocs.io/en/latest/lazy.html claims "This feature is fully implemented".

@GreyCat
Copy link
Member

GreyCat commented Jul 10, 2024

Folks, I can also point to the elephant in the room — do we have capacity/interest to even support Construct target?

Looks like @arekbulski unfortunately paused his participation in the project, so maybe we should follow the suit and disable/remove Construct support?

@generalmimon
Copy link
Member

@GreyCat:

Looks like @arekbulski unfortunately paused his participation in the project

In what project, Construct or KS? He seems to be still somewhat active in Construct: construct/construct#1080 (comment)

I assume Construct is not dead, i.e. it is probably still used by people. https://pypistats.org/packages/construct can give some very rough idea (although you never know how much of that is bot traffic).

The development of Construct might be stopped, but according to #377 (comment), that's supposedly because the project is "finished", which isn't inherently bad.

so maybe we should follow the suit and disable/remove Construct support?

I don't see any good reason to do that. In my view, "supporting" the Construct target doesn't cost us anything we don't want. We can keep it as it is now and some people may still find it useful. And if someone wants, they can improve it. Not that long ago, there were some some PRs by @Mimickal (https://github.com/kaitai-io/kaitai_struct_compiler/pulls?q=is%3Apr+is%3Aopen+construct+sort%3Aupdated-desc), which we haven't gotten to yet, but they show interest, and also @wbarnha seemed interested at some point (kaitai-io/kaitai_struct_compiler#242 (comment)). Also kaitai-io/kaitai_struct_compiler#256 by @MatrixEditor (which I assume was closed for the lack of our response).

(Speaking of hot candidates for removal, I think HtmlClassCompiler (-t html) should go first. The idea is OK, but the implementation is so preliminary that it arguably can't be useful to anyone.)

@Mimickal
Copy link

For some context, I was contributing to Construct support because at the time it was the only way to achieve serialization with Kaitai Struct. I've been away from the project that needed that for some time, but I've been keeping an eye on the space. As far as I can tell, there still isn't a great solution for data-driven struct generation like KSC anywhere.

Aside from the dev time needed to support it, I don't think there's a good case for removing Construct support from Kaitai until Kaitai's own Python generation supports serialization. Then at least there would be an alternative.

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

No branches or pull requests

4 participants