Add Lua rockspec package handler for LuaRocks dependencies#4743
Add Lua rockspec package handler for LuaRocks dependencies#4743SLASH217 wants to merge 19 commits intoaboutcode-org:developfrom
Conversation
Implement base handler class and Lua rockspec parser using luaparser for AST-based dependency extraction. Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
Extract package metadata and dependencies from rockspec files with version specification parsing. Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
* Test parser with real Kong rockspec file * Test handler integration with patterns Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
Register handler in APPLICATION_PACKAGE_DATAFILE_HANDLERS to enable automatic .rockspec file detection. Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
* Update contributing-docs link to new structure * Ensure both documentation links work correctly * Point to getting-started/contribute path Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
35fb9e2 to
c84c872
Compare
Change luaparser from == to match project's version pinning pattern: * requirements.txt: luaparser==1.4.3 (exact pinning) * setup.cfg: luaparser == 1.4.3 (exact pinning) Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
c84c872 to
3b359de
Compare
Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
- This was causing a test to fail in CI/CD Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
- Extra "/n" causing failure of CI/CD Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
AyanSinhaMahapatra
left a comment
There was a problem hiding this comment.
Thanks++ @SLASH217 for the PR, looking good already, but you need to modify the tests so we can see the full output and verify the functionality added before I can do an in depth review. Added some other feedback you also need to address.
| parser = rockspec.RockspecParser(test_file) | ||
| data = parser.parse() | ||
|
|
||
| assert data['rockspec_format'] == '3.0' |
There was a problem hiding this comment.
This is not how we write tests for package manifest parsers, asserting the data one by one. Instead we dump the whole results in a JSON file and check the output matches to it, so we can see the entire output and test it, and not just some fields. And this result is more readable, and for changes we can regen and check the changes in output.
This is done using tests.packagedcode.packages_test_utils.Packagetester.check_packages_data()
See how this is done in all other test files for package manifest parsers, for example:
| build = { | ||
| type = "builtin", | ||
| modules = { | ||
| ["kong"] = "kong/init.lua", |
There was a problem hiding this comment.
AFAIK we are not parsing/storing the modules info here and this is increasing the test file size without contributing anything to the test, can you remove these modules from all the tests?
There was a problem hiding this comment.
Same for all non essential things in other test files.
There was a problem hiding this comment.
I have removed the unnecessary bloat from the test files.
| @@ -0,0 +1,530 @@ | |||
| package = "kong" | |||
There was a problem hiding this comment.
Would be great to add the link to this test file (and all others) as a comment, since these are real files(?) and we should provide where we got them from
There was a problem hiding this comment.
Added the source of the rockspec files in test_file_source.txt.
| - name: Package name | ||
| - version_number: Clean version number (e.g. "3.1.3") or None | ||
| - version_spec: Full version specification with operator (e.g. "== 3.1.3") or None | ||
| - raw: Original input string |
There was a problem hiding this comment.
Not sure why you want to pass back the input, don't we already have this in context?
| package_name: Name of the package | ||
| package_version: Optional version string (without operators) | ||
|
|
||
| Returns: |
There was a problem hiding this comment.
This is additional information you're conveying twice, and not how we write the docstrings. Just describe the output, what it does and the inputs in a short to-the-point docstring. For all the functions
| extra_data['supported_platforms'] = platforms | ||
|
|
||
| # Future fields can be added here | ||
| # e.g., build_backend, build_requires, etc. |
There was a problem hiding this comment.
Why not just add these now? And if you're keeping work for later, add a TODO:
| jsonstreams >= 0.5.0 | ||
| license_expression >= 30.4.4 | ||
| lxml >= 5.4.0 | ||
| luaparser == 4.0.0 |
There was a problem hiding this comment.
Let's not pin dependencies, you can add this as a lower bound. We are a library too :)
There was a problem hiding this comment.
Understood! Will keep in mind for future commits.
|
Thank you for your feedback and valuable time. |
8bf9473 to
707adbc
Compare
Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
707adbc to
127f3fc
Compare
-- Added links for source of .rockspec files used in testing. Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
-- Modules not required to be parsed Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
-- updated to be lower bound Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
-- use check_packages_data -- instead of manually asserting data one by one. Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
-- Causing the CI/CD to fail. Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
127f3fc to
21ca2ff
Compare
Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
Signed-off-by: Prashanna Dahal <prashanna217@gmail.com>
|
@AyanSinhaMahapatra I have updated the PR, adding all the fixes you have requested. |
AyanSinhaMahapatra
left a comment
There was a problem hiding this comment.
Thanks @SLASH217, almost there. A couple small changes suggested for your consideration.
Could you also check if #3249 is fixed by this? See #3249 (comment) for the input.
|
|
||
| description = { | ||
| summary = "A fast JSON encoding/parsing module", | ||
| detailed = [[ |
There was a problem hiding this comment.
Please also parse and append the detailed description.
| @@ -0,0 +1,11 @@ | |||
| Test File : Source | |||
There was a problem hiding this comment.
Would be best to include this in the .rockspec files itself, as comments are supported.
-- This is a single-line comment
--[[
This is a multi-line comment
describing the build process.
]]
| @@ -0,0 +1,11 @@ | |||
| Test File : Source | |||
|
|
|||
| 1. test.rockspec : https://github.com/openresty/lua-cjson/blob/master/lua-cjson-2.1.0.16-1.rockspec | |||
There was a problem hiding this comment.
instead of test in filenames, use the package names, this is more verbose.
| if __name__ == '__main__': | ||
| import pytest | ||
| pytest.main([__file__, '-v']) |
There was a problem hiding this comment.
not required, please remove.
| }, | ||
| { | ||
| "purl": "pkg:luarocks/luaxxhash@1.0", | ||
| "extracted_requirement": ">= 1.0", |
There was a problem hiding this comment.
In case where the extracted requirement is not pinned, we should not include the version in the purl. As this is not the resolved version, only a requirement, actual version may be different.
| assert len(handlers) == 1, f"Expected 1 RockspecHandler, found {len(handlers)}" | ||
| assert handlers[0] == rockspec.RockspecHandler | ||
|
|
||
| def test_handler_in_datasource_registry(self): |
There was a problem hiding this comment.
How is this test really different from the test above, is this checking something new?
HANDLER_BY_DATASOURCE_ID is computed from the handlers lists
| luaparser==4.0.0 | ||
| MarkupSafe==3.0.3 | ||
| more-itertools==10.8.0 | ||
| multimethod==2.0.2 |
There was a problem hiding this comment.
scanned the new dependencies, looks safe to add
|
Understood! I will get to work as soon my exams end. |
Add Lua rockspec package handler for LuaRocks dependencies
Fixes #3526
Changes
RockspecHandlerclass for .rockspec file detectionRockspecParserusing Lua AST parsing with luaparser libraryAPPLICATION_PACKAGE_DATAFILE_HANDLERSregistryTasks
Run tests locally to check for errors.
Signed-off-by: Prashanna Dahal prashanna217@gmail.com