[pull] master from kaitai-io:master #3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
See #51 for how Ruby 2.4 was determined to be the target version.
Unify and change color palette for both ANSI and Windows console
The base `ruby` image dropped Debian 10 ("buster") images and converted them to Debian 12 ("bookworm") - see <docker-library/ruby#415>. However, while OpenJDK 11 was available in "buster", it is no longer available in "bookworm", so we now have to use OpenJDK 17 instead. …ntime Show Ruby runtime lib version in `{ksv,ksdump} --version` In most cases, the `$0` variable (note: the `$PROGRAM_NAME` used in the old code is an alias of `$0` in Ruby) works fine. However, for some unusual ways of launching our scripts, it doesn't because it essentially asks the wrong question: we don't really care what was the entry point of the current process (which is what `$0` means), we only want to get the directory of the current script so we can resolve relative paths from it. In particular, the old way didn't work when I launched `bin/ksdump` via https://github.com/ruby-prof/ruby-prof in order to profile it (for example as `ruby-prof -f ruby_prof.log bin/ksdump -- -f json sample.bin format.ksy > sample.json`), because `$0` in my case was `"C:/Ruby32-x64/bin/ruby-prof"`, which is obviously not what `bin/ksdump` expected.
There's no point in calling `public_methods` with no arguments first (this method is defined as `public_methods(all=true)`, so the `all` parameter is true by default), which exactly includes methods from *all* ancestor classes, only to remove all methods from ancestor classes manually using `Set` subtraction afterwards. It's much easier to set the `all` parameter to false in the first place, which will directly get us what we want. Not to mention that after doing some profiling with https://github.com/ruby-prof/ruby-prof of `bin/ksdump` on some big file, the `Set` manipulation was apparently quite slow as well, which really adds up for hundreds of millions of objects, for example. Note that even though a .ksy file typically only defines a handful of types, so there is only a handful of distinct classes in the generated parser module and the set of methods is the same for all objects of each class, the code in ksv doesn't take advantage of this and always enumerates methods for every object it sees as if it hadn't seen any object of the same class before. This is fine for simplicity and we don't have any evidence that "caching" of the set of methods per class would help, but the point is that you ideally want to avoid slow ways of doing things there, which the old way of enumerating methods using `Set` subtraction was.
…ave common pattern, added Gemfile to straightforward installation of dev deps
…cally for test classes mimicking KS generation
Maybe there is a deadlock?
It seems that the reason RuboCop was always hanging in CI was that it was stuck scanning the entirety of the massive `./vendor/bundle` directory with over 2000 files where the https://github.com/ruby/setup-ruby action stores the gems installed using `bundle install`. By default, that wouldn't happen because `vendor/**/*` is in the default `AllCops/Exclude` list (see https://github.com/rubocop/rubocop/blob/v1.79.1/config/default.yml#L68), but we overrode it with our own array. It is possible to set up [inheritance](https://docs.rubocop.org/rubocop/1.79/configuration.html#merging-arrays-using-inherit_mode) so that the `Exclude` arrays get merged, but for now it's easier to just remove our custom `Exclude` array, since there are no `spec/compiled/*.rb` files in the repo yet.
See https://github.com/kaitai-io/kaitai_struct_webide/wiki/Features#webide-representation (Web IDE does not display `to-string` yet, but it does display `-webide-representation`, and `to-string` is its successor, see kaitai-io/kaitai_struct#732) Related to kaitai-io/kaitai_struct#732
I believe it better expresses the intent. See https://docs.ruby-lang.org/en/master/String.html#class-String-label-Freezing-2FUnfreezing: > * `+@`: Returns a string that is not frozen: `self` if not frozen; > `self.dup` otherwise.
OpenJDK 17 is no longer available in Debian 13 (Trixie), see https://packages.debian.org/search?suite=trixie&searchon=names&keywords=-jre-headless
Related to kaitai-io/kaitai_struct_ruby_runtime#12 ```console $ bundle exec rspec --force-color ..........F. Failures: 1) Kaitai::Struct::Visualizer::Parser#load handles nil usage in size in seq Failure/Error: @data._read TypeError: no implicit conversion from nil to integer # ./vendor/bundle/ruby/3.4.0/gems/kaitai-struct-0.11/lib/kaitai/struct/struct.rb:365:in 'Kaitai::Struct::Stream#read_bytes' # /tmp/d20250912-3425-5mb57z/rely_on_nil.rb:31:in 'RelyOnNil#_read' # ./lib/kaitai/struct/visualizer/parser.rb:32:in 'Kaitai::Struct::Visualizer::Parser#load' # ./spec/parser_spec.rb:69:in 'block (3 levels) in <module:Visualizer>' ```
AFAIK all modern editors/IDEs (and even CLI editors such as `vim` or `nano`) use 1-based columns. The only editor using 0-based columns that I know of is GNU Emacs.
Previously, the error was reported as follows: ``` yaml_dup_keys.ksy:!:!: found duplicate key seq ```
See https://docs.ruby-lang.org/en/3.4/Open3.html#method-c-capture3: > Ruby invokes the executable directly, with no shell and no shell > expansion: > > ```ruby > Open3.capture3('doesnt_exist') # Raises Errno::ENOENT > ```
It probably makes more sense to have `ksdump --version` display `kaitai-struct-visualizer 0.7` rather than `ksdump 0.7`, because `ksdump` is not distributed separately, but only in the https://rubygems.org/gems/kaitai-struct-visualizer package. Also, let's be consistent and not use abbreviations in the `--help` text.
This eliminates duplication, which actually caused inconsistent behavior between handling errors from initial parsing vs. explicit parsing of lazy instances.
These methods were apparently copied from lib/kaitai/tui.rb and were not actually called anywhere (only the original versions in the Kaitai::TUI class are used).
This was pointed out by the `gem build` command when using RubyGems 3.7.2 on Linux: ```console $ gem build kaitai-struct-visualizer.gemspec WARNING: bin/ksdump is not executable WARNING: See https://guides.rubygems.org/specification-reference/ for help Successfully built RubyGem Name: kaitai-struct-visualizer Version: 0.7 File: kaitai-struct-visualizer-0.7.gem ``` Indeed - `bin/ksv` had the executable bit set, but `bin/ksdump` did not: ```console $ git ls-files -s bin/ 100644 9e4cafb 0 bin/ksdump 100755 b9024d7 0 bin/ksv ```
As in the runtime library, see kaitai-io/kaitai_struct_ruby_runtime@45ce0c3 Furthermore, these files were packed in https://rubygems.org/gems/kaitai-struct-visualizer/versions/0.7, and I don't think there's a good reason to remove them.
https://rubygems.org/gems/activesupport is the obvious one, otherwise `require 'active_support'` in bin/ksdump won't work. Then it turned out that we also need https://rubygems.org/gems/builder, otherwise we get this error: ```console $ ksdump -f xml pnggrad8rgb.png png.ksy You don't have builder installed in your application. Please add it to your Gemfile and run bundle install <internal:C:/Ruby34-x64/lib/ruby/3.4.0/rubygems/core_ext/kernel_require.rb>:136:in 'Kernel#require': cannot load such file -- builder (LoadError) Did you mean? bundler from <internal:C:/Ruby34-x64/lib/ruby/3.4.0/rubygems/core_ext/kernel_require.rb>:136:in 'Kernel#require' from C:/Ruby34-x64/lib/ruby/gems/3.4.0/gems/activesupport-8.0.2.1/lib/active_support/builder.rb:4:in '<top (required)>' from <internal:C:/Ruby34-x64/lib/ruby/3.4.0/rubygems/core_ext/kernel_require.rb>:136:in 'Kernel#require' from <internal:C:/Ruby34-x64/lib/ruby/3.4.0/rubygems/core_ext/kernel_require.rb>:136:in 'Kernel#require' from C:/Ruby34-x64/lib/ruby/gems/3.4.0/gems/activesupport-8.0.2.1/lib/active_support/core_ext/hash/conversions.rb:75:in 'Hash#to_xml' from C:/Ruby34-x64/lib/ruby/gems/3.4.0/gems/kaitai-struct-visualizer-0.11/bin/ksdump:89:in '<top (required)>' from C:/Ruby34-x64/bin/ksdump:36:in 'Kernel#load' from C:/Ruby34-x64/bin/ksdump:36:in '<main>' ```
…sts" This reverts commit 4b8f171.
…ping requires Ruby 2.5+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot]
Can you help keep this open source service alive? 💖 Please sponsor : )