-
Notifications
You must be signed in to change notification settings - Fork 197
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
Ruby, Lua: missing require
s not detected by tests due to require
leaks
#1099
Comments
See the source code of the KST translator - the `imports` list in .kst specs is copied to the `extraImports` property of the `TestSpec` case class, and `extraImports` are only used to generate `import` statements into generated test specs in C++, Java, Nim and Ruby. This means `import` statements in test specs don't need to duplicate `meta/imports`, it's enough to have extra imports only for specs that are referenced from expressions in `asserts` (for example, `imports: enum_0` is needed in `enum_import.kst` because there is an assertion with `expected: enum_0::animal::cat`). Since `enum_import.kst` is the only spec that refers from `asserts` to any of the specs listed in `imports`, this commit removes the unnecessary `imports` from all other .kst specs. This partly addresses <kaitai-io/kaitai_struct#1099>. While this doesn't solve the root of the problem, at least now tests that import specs that are not tested directly and also not imported from any other spec (or if they are, then only from a test that comes later in the alphabet and thus has not run yet) no longer pass. This means that with the current state of KSC, this breaks 5 tests in Ruby: * CastToImported * EnumToIClassBorder1 * ImportsAbs * ImportsCircularA * ImportsRel1 The fact that these tests now fail is a good thing, because ultimately, we want to ensure that all generated Ruby format modules contain the `require` statements they themselves need to function, which is not the case right now, so they *should* be failing. Ironically, I believe that the `imports` in KST specs removed in this commit were in place specifically to make Ruby tests pass (because AFAIK, they are unnecessary from the perspective of any other language). However, this reliance of Ruby format modules on external `require`s is incorrect.
The same problem occurs in Lua - -- This is a generated file! Please edit source .ksy file and use kaitai-struct-compiler to rebuild
--
-- This file is compatible with Lua 5.3
local class = require("class")
require("kaitaistruct")
EnumImport = class.class(KaitaiStruct)
function EnumImport:_init(io, parent, root)
KaitaiStruct._init(self, io)
self._parent = parent
self._root = root or self
self:_read()
end
function EnumImport:_read()
self.pet_1 = Enum0.Animal(self._io:read_u4le())
self.pet_2 = EnumDeep.Container1.Container2.Animal(self._io:read_u4le())
end Unlike Ruby, -- Autogenerated from KST: please remove this line if doing any edits by hand!
local luaunit = require("luaunit")
-- require("enum_0")
TestEnum0 = {}
function TestEnum0:test_enum_0() ... and as long as it present,
|
require
s not detected by tests due to require
leaksrequire
s not detected by tests due to require
leaks
See the source code of the KST translator - the `imports` list in .kst specs is copied to the `extraImports` property of the `TestSpec` case class, and `extraImports` are only used to generate `import` statements into generated test specs in C++, Java, Nim and Ruby. This means `import` statements in test specs don't need to duplicate `meta/imports`, it's enough to have extra imports only for specs that are referenced from expressions in `asserts` (for example, `imports: enum_0` is needed in `enum_import.kst` because there is an assertion with `expected: enum_0::animal::cat`). Since `enum_import.kst` is the only spec that refers from `asserts` to any of the specs listed in `imports`, this commit removes the unnecessary `imports` from all other .kst specs. This partly addresses <kaitai-io/kaitai_struct#1099>. While this doesn't solve the root of the problem, at least now tests that import specs that are not tested directly and also not imported from any other spec (or if they are, then only from a test that comes later in the alphabet and thus has not run yet) no longer pass. This means that with the current state of KSC, this breaks 4 tests in Ruby: * EnumToIClassBorder1 * ImportsAbs * ImportsCircularA * ImportsRel1 The fact that these tests now fail is a good thing, because ultimately, we want to ensure that all generated Ruby format modules contain the `require` statements they themselves need to function, which is not the case right now, so they *should* be failing. Ironically, I believe that the `imports` in KST specs removed in this commit were in place specifically to make Ruby tests pass (because AFAIK, they are unnecessary from the perspective of any other language). However, this reliance of Ruby format modules on external `require`s is incorrect.
See the source code of the KST translator - the `imports` list in .kst specs is copied to the `extraImports` property of the `TestSpec` case class, and `extraImports` are only used to generate `import` statements into generated test specs in C++, Java, Nim and Ruby. This means `import` statements in test specs don't need to duplicate `meta/imports`, it's enough to have extra imports only for specs that are referenced from expressions in `asserts` (for example, `imports: enum_0` is needed in `enum_import.kst` because there is an assertion with `expected: enum_0::animal::cat`). Since `enum_import.kst` is the only spec that refers from `asserts` to any of the specs listed in `imports`, this commit removes the unnecessary `imports` from all other .kst specs. This partly addresses <kaitai-io/kaitai_struct#1099>. While this doesn't solve the root of the problem, at least now tests that import specs that are not tested directly and also not imported from any other spec (or if they are, then only from a test that comes later in the alphabet and thus has not run yet) no longer pass. This means that with the current state of KSC, this breaks 4 tests in Ruby: * EnumToIClassBorder1 * ImportsAbs * ImportsCircularA * ImportsRel1 The fact that these tests now fail is a good thing, because ultimately, we want to ensure that all generated Ruby format modules contain the `require` statements they themselves need to function, which is not the case right now, so they *should* be failing. Ironically, I believe that the `imports` in KST specs removed in this commit were in place specifically to make Ruby tests pass (because AFAIK, they are unnecessary from the perspective of any other language). However, this reliance of Ruby format modules on external `require`s is incorrect.
See the source code of the KST translator - the `imports` list in .kst specs is copied to the `extraImports` property of the `TestSpec` case class, and `extraImports` are only used to generate `import` statements into generated test specs in C++, Java, Nim and Ruby. This means `import` statements in test specs don't need to duplicate `meta/imports`, it's enough to have extra imports only for specs that are referenced from expressions in `asserts` (for example, `imports: enum_0` is needed in `enum_import.kst` because there is an assertion with `expected: enum_0::animal::cat`). Since `enum_import.kst` is the only spec that refers from `asserts` to any of the specs listed in `imports`, this commit removes the unnecessary `imports` from all other .kst specs. This partly addresses <kaitai-io/kaitai_struct#1099>. While this doesn't solve the root of the problem, at least now tests that import specs that are not tested directly and also not imported from any other spec (or if they are, then only from a test that comes later in the alphabet and thus has not run yet) no longer pass. This means that with the current state of KSC, this breaks 4 tests in Ruby: * EnumToIClassBorder1 * ImportsAbs * ImportsCircularA * ImportsRel1 The fact that these tests now fail is a good thing, because ultimately, we want to ensure that all generated Ruby format modules contain the `require` statements they themselves need to function, which is not the case right now, so they *should* be failing. Ironically, I believe that the `imports` in KST specs removed in this commit were in place specifically to make Ruby tests pass (because AFAIK, they are unnecessary from the perspective of any other language). However, this reliance of Ruby format modules on external `require`s is incorrect.
See the source code of the KST translator - the `imports` list in .kst specs is copied to the `extraImports` property of the `TestSpec` case class, and `extraImports` are only used to generate `import` statements into generated test specs in C++, Java, Nim and Ruby. This means `import` statements in test specs don't need to duplicate `meta/imports`, it's enough to have extra imports only for specs that are referenced from expressions in `asserts` (for example, `imports: enum_0` is needed in `enum_import.kst` because there is an assertion with `expected: enum_0::animal::cat`). Since `enum_import.kst` is the only spec that refers from `asserts` to any of the specs listed in `imports`, this commit removes the unnecessary `imports` from all other .kst specs. This partly addresses <kaitai-io/kaitai_struct#1099>. While this doesn't solve the root of the problem, at least now tests that import specs that are not tested directly and also not imported from any other spec (or if they are, then only from a test that comes later in the alphabet and thus has not run yet) no longer pass. This means that with the current state of KSC, this breaks 4 tests in Ruby: * EnumToIClassBorder1 * ImportsAbs * ImportsCircularA * ImportsRel1 The fact that these tests now fail is a good thing, because ultimately, we want to ensure that all generated Ruby format modules contain the `require` statements they themselves need to function, which is not the case right now, so they *should* be failing. Ironically, I believe that the `imports` in KST specs removed in this commit were in place specifically to make Ruby tests pass (because AFAIK, they are unnecessary from the perspective of any other language). However, this reliance of Ruby format modules on external `require`s is incorrect.
... instead of expecting the calling code to load everything that the generated modules need. Fixes these tests for Ruby: * ImportsCastToImported * ImportsCastToImported2 * ImportsParamsDefEnumImported * ImportsParamsDefUsertypeImported We intentionally use `require_relative` (instead of `require` as usual) and only for imported types (not opaque types) - see the added code comment in `RubyCompiler`'s implementation of `externalTypeDeclaration` for more details. Until now, we've been kind of living under the impression that the generated Ruby files don't need explicit imports, because until recently, all of our import-related tests in https://github.com/kaitai-io/kaitai_struct_tests were passing despite the fact that KSC has never generated any `require`s in Ruby. However, it turned out that this was just a coincidence described in kaitai-io/kaitai_struct#1099, caused by the fact that the Ruby's `require` pollutes the global namespace and that the imported specs for which we were missing `require`s were already required by some other test. This was not the case in the 4 tests that this commit fixes as mentioned above, which were added recently and all import a format spec that no other test uses. This "missing `require`s" problem would have been an issue in [KSV](https://github.com/kaitai-io/kaitai_struct_visualizer) too, but KSV works around it by requiring every single `*.rb` file that KSC has generated (which will include any imported .ksy specs), and opaque types must be explicitly imported using the `-r` command-line option. This should still work after this commit, although calling `require` with each `*.rb` file in the directory of generated format modules becomes unnecessary.
I was wondering how it is possible that the
EnumImport
test passes in Ruby (see https://ci.kaitai.io/ orci.json:239-243
of theruby/3.3-linux-x86_64
target).The
enum_import.ksy
spec imports two other .ksy specs (enum_0
andenum_deep
) and defines twoseq
fields, each of an enum type defined in one of the imported specs:The problem is that the
enum_import.rb
(still generated by the latest KSC at the time of writing, i.e.29f7a592
) looks like this:Notice that it refers to classes
Enum0
andEnumDeep
on these lines:... but there are no corresponding
require
statements, so the Ruby interpreter should have no idea what these names are. And yet this test passes in Ruby.It turns out that the
require
s in Ruby are "leaky", i.e. they pollute the global namespace. So if any format or test Ruby files running before the aboveenum_import.rb
containrequire 'enum_0'
andrequire 'enum_deep'
(which is what theEnumImport
class needs to run successfully) in the same Ruby interpreter process, the fact that theenum_import.rb
file doesn't itself have therequire
s it needs is not detected by tests.And if you search for
require 'enum_0'
and forrequire 'enum_deep'
, you can see that bothrequire
s have 2 occurrences in test specs.If you disable the
enum_0
inspec/ruby/enum_import_spec.rb
:... it's not enough (i.e. the
EnumImport
test will still pass), becauseenum_0
is also tested directly, sorequire 'enum_0'
is also inspec/ruby/enum_0_spec.rb
:EnumImport
will finally fail (as it should have all this time) only after you disable both theserequire
s:The text was updated successfully, but these errors were encountered: