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

Add support for rpm embedded lua interpreter #66

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

moedan
Copy link
Contributor

@moedan moedan commented Aug 29, 2024

Hi,

this is a first draft to support the embedded Lua interpreter of RPM by using <lua> as interpreter. See #101 in rpm-builder.

I'm not that deep into this project yet, so suggestions for improvement are welcome.

A question for better understanding:
Why are the requirements for the interpreter are added twice (with different flags) in the set*Script-Methods?
E.g.:

addRequirement(interpreter, null, RpmDependencyFlags.INTERPRETER);
addRequirement(interpreter, null, RpmDependencyFlags.SCRIPT_PRE, RpmDependencyFlags.INTERPRETER);

@ctron
Copy link
Contributor

ctron commented Aug 29, 2024

I guess you need to sign the Eclipse CLA. Let me know if you need any help with that.

The PR looks good to me. I guess there could be bonus points for making this a hash map, allowing for something like: <lua> -> "requirements". Making this a bit more future proof.

But only if you think it's the right idea and you want to do it.

@dwalluck
Copy link
Contributor

But only if you think it's the right idea and you want to do it.

https://github.com/rpm-software-management/rpm/blame/671fc8e5d6633c14c9621903a23c0040acb43f65/lib/rpmds.c#L1086

says that it hasn't changed in 17 years although RPM 4.2.2 was released in 2004 which makes it 20 years old. Remember that this is the minimum requirement (usually the version that it was introduced in).

@dwalluck
Copy link
Contributor

The PR looks good to me.

Maybe a test should be added?

@moedan
Copy link
Contributor Author

moedan commented Sep 2, 2024

I guess you need to sign the Eclipse CLA.

Thanks for the info. I'm currently in dialogue with my company's legal department about this. It will probably take some days before I get an answer.

Maybe a test should be added?

Yes, I'm going to add a test.

@moedan
Copy link
Contributor Author

moedan commented Sep 9, 2024

Good news: I have signed the ECA. =)
I've also added a few tests. However, it was a little more complicated than I thought. In the end, I orientated myself on the existing WriterTests for the implementation.

@ctron ctron merged commit eeb740e into eclipse:master Sep 10, 2024
4 checks passed
@ctron
Copy link
Contributor

ctron commented Sep 10, 2024

Any more changes? Or should I start working on a new release?

@moedan
Copy link
Contributor Author

moedan commented Sep 10, 2024

Thanks for the quick response!

After implementing the tests, it is still not entirely clear to me why, for example, two required entries are generated for a %post-shell-scriptlet entry.

E.g. RpmBuilder::setPostInstallationScript("my scrip") results in following required dependencies (Output from Dumper):

Require: /bin/sh -  - 256 [INTERPRETER]
Require: /bin/sh -  - 1280 [INTERPRETER, SCRIPT_POST]
Require: rpmlib(CompressedFileNames) - 3.0.4-1 - 16777226 [LESS, EQUAL, RPMLIB]
Require: rpmlib(PayloadFilesHavePrefix) - 4.0-1 - 16777226 [LESS, EQUAL, RPMLIB]

From my limited understanding, one entry for /bin/sh should be sufficient, e.g.:

Require: /bin/sh -  - 1280 [INTERPRETER, SCRIPT_POST]
Require: rpmlib(CompressedFileNames) - 3.0.4-1 - 16777226 [LESS, EQUAL, RPMLIB]
Require: rpmlib(PayloadFilesHavePrefix) - 4.0-1 - 16777226 [LESS, EQUAL, RPMLIB]

It would be helpful for me if you could explain why it is implemented in the rpm pakgager the way it is.


It is interesting to see how rpmbuild behaves here. If I build a minimal rpm with rpmbuild, containing a shell and a lua scriptlet, then the following required dependencies are set:

$ rpmbuild --version
RPM version 4.19.1.1

$ rpmbuild -bb mwe.spec

$ rpm -q --queryformat "[%{REQUIRENAME} %{REQUIREFLAGS} %{REQUIREVERSION}\n]" mwe-1.0-1.x86_64.rpm 
/bin/sh 1280 
rpmlib(BuiltinLuaScripts) 16777226 4.2.2-1
rpmlib(CompressedFileNames) 16777226 3.0.4-1
rpmlib(FileDigests) 16777226 4.6.0-1
rpmlib(PayloadFilesHavePrefix) 16777226 4.0-1
rpmlib(PayloadIsZstd) 16777226 5.4.18-1

Flags according to RPMDependenyFlags::parse:
1280 -> INTERPRETER, SCRIPT_POST
16777226 -> LESS, EQUAL, RPMLIB

Used mwe.spec:

Name:           mwe
Release:        1
Summary:        Minimal Working Example
License:        none
Version:        1.0

%description
My minimal working example

%files

%pre -p <lua>
print('Hello from Lua')

%post
echo 'Hello from shell'

@ctron
Copy link
Contributor

ctron commented Sep 10, 2024

It would be helpful for me if you could explain why it is implemented in the rpm pakgager the way it is.

TBH, no I can't 🤣 … it's been a while. I am not sure it hurts. It might be ok to remove the additional entry.

@moedan
Copy link
Contributor Author

moedan commented Sep 10, 2024

It might be ok to remove the additional entry.

I would like to remove them. Should I create a new PR for it?


Unfortunately, it looks like I made a small mistake with the required dependency flags for lua:
In other packages that use lua scriptlets (e.g. glibc and bash), only the flags LESS, EQUAL, RPMLIB (16777226) are set for the required dependency rpmlib(BuiltinLuaScripts).
I have not yet found any other examples that have set other flags for this required dependency. But I also have not found a rule for this in the RPM Reference Manual yet.
Anyway, in order to have it consistent with the behaviour of the rpmbuilder and other packages, I would like to adjust this, if nothing speaks against it.

@ctron
Copy link
Contributor

ctron commented Sep 10, 2024

It might be ok to remove the additional entry.

I would like to remove them. Should I create a new PR for it?

Unfortunately, it looks like I made a small mistake with the required dependency flags for lua: In other packages that use lua scriptlets (e.g. glibc and bash), only the flags LESS, EQUAL, RPMLIB (16777226) are set for the required dependency rpmlib(BuiltinLuaScripts). I have not yet found any other examples that have set other flags for this required dependency. But I also have not found a rule for this in the RPM Reference Manual yet. Anyway, in order to have it consistent with the behaviour of the rpmbuilder and other packages, I would like to adjust this, if nothing speaks against it.

Makes sense. Both topics!

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

Successfully merging this pull request may close these issues.

3 participants