Page MenuHomeFreeBSD

Lua version of the @sample
AbandonedPublic

Authored by bapt on Feb 11 2020, 9:49 AM.

Details

Reviewers
manu
Group Reviewers
portmgr
O5: Ports Framework(Owns No Changed Paths)
Commits
rP550860: Lua version of the @sample
Summary

The bonus of this version being: sandboxed
Natively rootdir compliant.

Test Plan

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 29287
Build 27197: arc lint + arc unit

Event Timeline

Are the permissions handled ? (owner/group/mode)
Previously they were handled by the cp -p

Are the permissions handled ? (owner/group/mode)
Previously they were handled by the cp -p

Good catch, I haven't thought about that one. and the simple answer is no they are not.

Mmmm, I'm sorry, but how is this better?
It add like 15 lines to the file, and now I absolutely cannot read what it does.

bapt planned changes to this revision.Apr 29 2020, 2:02 PM
manu updated this revision to Diff 74687.
manu added a reviewer: bapt.

Switch to a smaller version that uses the latest pkg lua functions addition,

This is, of course, intended to be commited when we release 1.15 in ports-mgmt/pkg

Are the permissions handled ? (owner/group/mode)
Previously they were handled by the cp -p

persmissions are preserved now, pkg.filecopy always preserve the owner/group/mode

In D23617#518945, @mat wrote:

Mmmm, I'm sorry, but how is this better?
It add like 15 lines to the file, and now I absolutely cannot read what it does.

Does it looks better now ?

I wonder about the design choices behind having "magic" strings everywhere, those %1, %@ and all.

It would be much more readable to have like, an array, say, pkg.args[1] and pkg.args[2], so you could know the number of arguments with pkg.args.length or pkg.n_args or something.

In D23617#569709, @manu wrote:

This is, of course, intended to be commited when we release 1.15 in ports-mgmt/pkg

Well, no, this can only be committed when 1.15 is the oldest version that is supported.

Keywords/sample.ucl
30

There is so much magic going on here.

What is %@ ? What is %S+ ? I looked at pkg-lua-script(5) and it does not appear in it.

36–38

This is not what the script was doing before.

The script copies the .sample to the destination file if the destination file does not exist. There is absolutely no comparison involved. There should be a pkg.fileexists or something.

In D23617#569765, @mat wrote:

I wonder about the design choices behind having "magic" strings everywhere, those %1, %@ and all.

It would be much more readable to have like, an array, say, pkg.args[1] and pkg.args[2], so you could know the number of arguments with pkg.args.length or pkg.n_args or something.

Those are expanded are package creation by the keywords framework.
There is not pkg lua object containing the args, it's replaced by the framework and the real value (passed to @sample) are put in the resulting lua script in the +MANIFEST file.

In D23617#569709, @manu wrote:

This is, of course, intended to be commited when we release 1.15 in ports-mgmt/pkg

Well, no, this can only be committed when 1.15 is the oldest version that is supported.

Do we support building ports from the head branch on a machine using quarterly packages ? That seems odd to me.
I don't mind waiting ~1 month between 1.15 is in ports-mgmt/pkg and this but waiting a full quarterly seems a bit much.

Keywords/sample.ucl
36–38

This is exactly what it does too.
Sure there is a "useless" cmp, I can see to add a fileexist (or use of the lua os. function maybe)

In D23617#569765, @mat wrote:

I wonder about the design choices behind having "magic" strings everywhere, those %1, %@ and all.

This is there since the begining of keywords, it is designed over the shell variables: $1 $2 $@ $# and do works the same, this is absolutely not anything which is related to lua at all. the usage of % for the variable name is based on the already existing since pkg_install (
expension variables: %F %B etc.

The expansion of %1 %2 %@ %# happens at the packaging time the the "variable" are not saved anywhere, so it is impossible to push them back as regular variables for the lua scripts (the same way we don't do it for shell).

Keywords/sample.ucl
30

I copied the gsub from the old review but a simple test on %# will be enough. I'll update the code.

Use pkg.stat instead of pkg.filecmp to check if the target file exists.
Use '%#' instead of gsub the '%@' variable.

manu marked 2 inline comments as done.Jul 21 2020, 8:21 AM

Looks ok. Could you indent the inside of the here documents one step so the only lines that begin with text are ucl?

I am accepting this as portmgr, I assume it works ;-)

We would make an exp-run just to be sure there is no elephant in the room

This revision is now accepted and ready to land.Aug 28 2020, 11:46 AM
This revision was automatically updated to reflect the committed changes.
mandree added a subscriber: mandree.

This is broken and causes leftover files in the poudriere testport security/ca_root_nss, see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=250415
Please revise.

There is also a bug on deinstall if a @sample keyword is used with only one argument that does NOT end in .sample: in that case, the configuration file is copied onto itself on install, and on deinstall, it is compared to itself, found matching, and removed. Arguably, this can also be caught in the stage-qa phase and would also be clobbered on update, but it should be flagged so that the offending package's pkg-plist can be fixed. I have seen and fixed such bugs in the wild.

bapt abandoned this revision.
bapt edited reviewers, added: manu; removed: bapt.