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

chore: Use modern Lua functions in chisels #1616

Merged

Conversation

federico-sysdig
Copy link
Contributor

What type of PR is this?
/kind cleanup

Any specific area of the project related to this PR?
/area build

Does this PR require a change in the driver versions?
No.

What this PR does / why we need it:
Replaces outdated functions of the library for embedding Lua with more modern ones. This will allow to upgrade the Lua library in the future. For instance Vcpkg, the Microsoft package manager, provides a more modern version of Lua that would need this change.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Some minor cleanup has been performed in the involved files.

Does this PR introduce a user-facing change?:
No.

NONE

incertum
incertum previously approved these changes Jan 11, 2024
Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/milestone 0.15.0

/approve

note for the second reviewer: not too knowledgeable about the chisels and our lua use, but just looking at the code changes totally LGTM

@poiana poiana added this to the 0.15.0 milestone Jan 11, 2024
@poiana poiana added the lgtm label Jan 11, 2024
@poiana
Copy link
Contributor

poiana commented Jan 11, 2024

LGTM label has been added.

Git tree hash: 83eaf67bd1e7110714440fbf448e12792830940e

@incertum
Copy link
Contributor

@federico-sysdig few CI hiccups, will re-approve after fixing them all.

@federico-sysdig
Copy link
Contributor Author

federico-sysdig commented Jan 12, 2024

@federico-sysdig few CI hiccups, will re-approve after fixing them all.

@incertum Thanks.
Yes, the CI builds with "non-bundled" libs use liblua5.1 which is not as updated as the LuaJIT of the bundled lib. I have updated the CI builds, this should hopefully fix them.

@poiana poiana added size/L and removed size/M labels Jan 12, 2024
Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@poiana poiana added the lgtm label Jan 12, 2024
@poiana
Copy link
Contributor

poiana commented Jan 12, 2024

LGTM label has been added.

Git tree hash: b064be47a29feb490b8035151bd460c8108dc984

Copy link
Contributor

@jasondellaluce jasondellaluce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@FedeDP
Copy link
Contributor

FedeDP commented Jan 15, 2024

This needs to be rebased on master to fix CI :)

@federico-sysdig federico-sysdig force-pushed the modernize-lua-for-chisels branch from 186f10a to 6ee42ca Compare January 15, 2024 15:44
@poiana poiana removed the lgtm label Jan 15, 2024
@federico-sysdig
Copy link
Contributor Author

This needs to be rebased on master to fix CI :)

@FedeDP Done.

Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@poiana
Copy link
Contributor

poiana commented Jan 15, 2024

LGTM label has been added.

Git tree hash: 08ade2a68fbc0382d2a7bd11523743ea2e6224b3

Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@poiana
Copy link
Contributor

poiana commented Jan 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, federico-sysdig, incertum, jasondellaluce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [FedeDP,incertum,jasondellaluce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@leogr
Copy link
Member

leogr commented Jan 16, 2024

Hey @federico-sysdig

This PR is blocked by this failing test 👇
CI Build / build-libs-linux-arm64 😁 (system_deps_minimal) (pull_request)

Could you take a look please? 🙏

Otherwise, LGTM.

@federico-sysdig
Copy link
Contributor Author

federico-sysdig commented Jan 16, 2024

This PR is blocked by this failing test 👇 CI Build / build-libs-linux-arm64 😁 (system_deps_minimal) (pull_request)
Could you take a look please? 🙏

@leogr I've looked and there is a test that's timing out. The test seems to be unrelated to the change and I don't recall it failing before rebasing (did it in order to be able to merge). Do we have experience of some tests failing erratically due to some other reason?

@poiana poiana merged commit 77dc7de into falcosecurity:master Jan 16, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants