interpreter: handle multiple dirs in install_emptydir

[Hi-Angel]

This fixes a bug where all but the first one directories passed to install_emptydir were ignored.

This adds a new feature that allows passing multiple paths to install_emptydir

Fixes: #12717

[dcbaker]

This is a feature change, there happens to be a bug that causes us to not error if you pass more than one argument, but the documentation is pretty clear that only a single argument is accepted, and the annotations are clear that only a single argument is expected.

So, this should be treated as a regular enhancement (though, lets please get the @typed_pos_args() decorator). Apart from the inline comments, we should also have a snippet in the release notes, and the documentation needs to be updated to reflect these changes.

[dcbaker]

We should be validating the inputs here:

@typed_pos_args('install_emptydir', varargs=str, min_varargs=1)

[Hi-Angel]

Done!

[dcbaker]

Suggested change

	def func_install_emptydir(self, node: mparser.BaseNode, args: T.Tuple[str], kwargs) -> None:
	def func_install_emptydir(self, node: mparser.BaseNode, args: T.Tuple[T.List[str]], kwargs) -> None:

[Hi-Angel]

Done!

[dcbaker]

We need to do something here to say that a accepting an array of arguments is a new feature. I would suggest something like:

if`len(args) > 1:
    FeatureNew.single_use(...)

[Hi-Angel]

Done!

[Hi-Angel]

but the documentation is pretty clear that only a single argument is accepted

Oh, I'm sorry, you are correct, I was confused all that time because:

1. The documentation declares it to accept str dirpath... which I misinterpreted as "variadic parameter that accepts one or more strings"
2. Passing multiple strings didn't error out

[Hi-Angel]

the annotations are clear that only a single argument is expected

So, for the record, unless I'm missing something, the type annotation was not correct, the yaml documentation file is mentioning that dirpath is a varargs.

I'm pointing that out mainly because I did not change that, so… just in case I am misunderstanding something.

[eli-schwartz]

Overall I wonder what the advantage is of specifying multiple arguments to install_emptydir vs. just invoking install_emptydir multiple times?

[eli-schwartz]

The tail is wagging the dog. The annotation disagrees with the code, so you change the code to align with the annotation?

Every other function here returns the build.* object it creates, and is annotated to do so.

[Hi-Angel]

I am not sure I understand what you mean… The annotation implies the return value is unused, so why have the excess code?

Every other function here returns the build.* object it creates, and is annotated to do so.

That's not true, func_subdir, func_add_test_setup, etc return nothing.

[eli-schwartz]

Sorry, I meant the other install_*() functions.

The annotation does NOT imply the return value is used, the annotation implies that the return value is None but that isn't actually true.

[Hi-Angel]

Ah, okay. I'm unclear though, if None return type is incorrect, which one is then? I presume that after this PR it ought to be List[build.EmptyDir], but this differs from other install_*() functions, and… like, nothing falls out due to changing the type: tests do not break, linter does not complain, Meson works as usual…

That's actually another reason why I believe that None is the correct type, because clearly nothing uses the return value of the function, so why have it return anything?

[Hi-Angel]

Should I perhaps change other install_*() functions to also return None…? I might be wrong as I did not test that, but offhand I would guess that nothing might be using their return values as well.

[eli-schwartz]

Not sure how much sense it makes to put an interdocumentation hyperlink into the header / hyperlink anchor.

[Hi-Angel]

In this case I was mostly following the style, e.g. if you open release notes for v1.3.0 you'll similarly find a configure_file inside a title that refers to the documentation.

That said, I think there is a practical reason: it's just comfortable for someone not knowing about a function to be able to just click it and follow to the docs.

[eli-schwartz]

Could be. I don't have strong feelings about it.

[eli-schwartz]

This is not the type of information that users upgrading meson care about or should be told. They'll get much more confused than the confusion this PR suggests to fix.

The release notes should say what feature is new.

[Hi-Angel]

Again, I'm just following the style. If you open release notes for v1.3.0 you'll similarly find that the first section "Clarify of implicitly-included headers in C-like compiler checks" explains also the reasoning behind the change. Is that bad?

[eli-schwartz]

The (large) difference is that the release notes for implicitly included headers in C-like compiler checks because they are important background information for users to know why the compiler checks now behave differently when used, and also for users to know how to adapt to that if needed.

Here, the "reasoning behind the change" is information about meson's type annotations.

If you're discussing the mypy-checked PEP 484 type hints in the release notes for a build system, you're doing "it" wrong.

But also, this release notes snippet just has too much information to sift through in order for users to find out what they care about. With the other snippet, users have to know about the change to stdio.h in order to know what they should do.

[Hi-Angel]

Done! I removed almost all explanation and only left

Prior to this release passing multiple paths as dirpath argument was resulting in only the first path to get created. Now passing more than one path in dirpath will create all of them.

[eli-schwartz]

Typically we'd say on a brand new line:

(since 1.4.0) multiple directories are accepted and will be installed.

[Hi-Angel]

Done!

[eli-schwartz]

This is a complicated way to write:

for one_dir in args[0]:
    d = build.EmptyDir(one_dir, kwargs['install_mode'], self.subproject, kwargs['install_tag'])
    self.build.emptydir.append(d)

... which is also the style used by other functions.

[Hi-Angel]

Will change if you want, but I should mention that I see nothing complicated in set-builder notation. AFAIK these set-comprehensions are pythonic way of coding. Though I personally don't consider myself a Python programmer that much, but I like the style because it's functional compared to one you suggested which is imperative.

[eli-schwartz]

It is a list comprehension, not a set.

And list comprehensions are an extremely powerful, valuable, amazing feature of python -- and should be treated with care as a result. Here, a list comprehension is being used with two different forms of line continuation because the thing being done is so long. This does not enhance readability IMO.

[Hi-Angel]

Done!

[dcbaker]

the annotations are clear that only a single argument is expected

So, for the record, unless I'm missing something, the type annotation was not correct, the yaml documentation file is mentioning that dirpath is a varargs.

I'm pointing that out mainly because I did not change that, so… just in case I am misunderstanding something.

The current annotations are:

def func_install_empty_dir(self, node: BaseNode, args: Tuple[str], kwargs: ...): ...

that means it expections something like ('foo', ), ie, a tuple with one string in it. What it is actually getting is List[str], and it happens to only use the 0th element. Fun fact, as-is, if you call install_emptydir([]), it will throw an uncaught IndexError. With the use of varargs, typed_pos_args will pass a Tuple[List[str]], a tuple containing one element, which is a list of strings. It does this since you can have functions like @typed_pos_args('myfunc', str, varargs=str, min_varargs=1), this is what project() does, for example.

[Hi-Angel]

@dcbaker sorry, by type annotation I meant not the ones in code but the ones in the yaml documentation, i.e. here it says it accepts a vararg. A subdir for example have posarg instead.

The type in the code I changed as you asked. What I was pointing out that I didn't change the type in yaml docs because I presume it does not require any changes.

[Hi-Angel]

So, everything seems addressed except the one comment where it's unclear what's required, there was no comment so far… Please review C:

[Hi-Angel]

So, is everything fine? Would be nice to have it merged before 1.4.0 release 😊

[eli-schwartz]

Overall I wonder what the advantage is of specifying multiple arguments to install_emptydir vs. just invoking install_emptydir multiple times?

Still interested in an answer here. Specifying multiple arguments to install_emptydir doesn't feel terribly ergonomic to me, which is why when I originally implemented it I didn't design it to work that way.

[Hi-Angel]

Overall I wonder what the advantage is of specifying multiple arguments to install_emptydir vs. just invoking install_emptydir multiple times?

Still interested in an answer here. Specifying multiple arguments to install_emptydir doesn't feel terribly ergonomic to me, which is why when I originally implemented it I didn't design it to work that way.

Oh, I am sorry for missing this.

Well, you are correct it is possible to just do a foreach cycle, which is exactly how I have it implemented at work right now, till #12717 resolved.

It's just that being able to pass a list of directories directly rather than have a foreach reduces the code a bit on the user-side; and given that implementing such capability amounts to just 6 changed lines and 8 more for tests, it seems like there's no maintenance burden to supporting this. Just a nice and easy feature to have 😊

[Hi-Angel]

From the user POV, it seems rational that if applying a function to more than one argument would make sense for the work it's supposed to do, then probably the function might accept more than one argument.

It was my thinking when I looked at the docs for install_emptydir and found that it claims to accept a variadic type argument, which totally made sense to me. Which then resulted in the report #12717, and then as it turned out here, it was simply wrong type of the argument in docs.

[Hi-Angel]

From the user POV, it seems rational that if applying a function to more than one argument would make sense for the work it's supposed to do, then probably the function might accept more than one argument.

It was my thinking when I looked at the docs for install_emptydir and found that it claims to accept a variadic type argument, which totally made sense to me. Which then resulted in the report #12717, and then as it turned out here, it was simply wrong type of the argument in docs.

As a matter of fact, IIRC passing a list to the function was the first thing I did, while refactoring our meson.builds to get rid of hardcoded mkdir. Looking at the docs was something I did afterwards, which in combination with varargs in the docs got me confused.

[eli-schwartz]

It's just that being able to pass a list of directories directly rather than have a foreach reduces the code a bit on the user-side; and given that implementing such capability amounts to just 6 changed lines and 8 more for tests, it seems like there's no maintenance burden to supporting this. Just a nice and easy feature to have 😊

Well, there's three possibilities here.

dirs = [
    '/usr/share/foo',
    '/usr/share/bar',
]

foreach d: dirs
    install_emptydir(d)
endforeach

install_emptydir(
    '/usr/share/foo',
    '/usr/share/bar',
)

install_emptydir('/usr/share/foo')
install_emptydir('/usr/share/bar')

The first one is something I'd never do at all.

The second one isn't an obvious win to me. I might use it if it existed... maybe. But I naturally gravitated to implementing option 3 only.

Option 3 takes fewer lines. Option 3 is also required if you need to specify different install_mode values, which I considered to be particularly likely at the time -- installing an empty directory is a very important use case when it's not even possible for the application to create the directory later on because it doesn't run as root but needs to use a dedicated application directory in /etc or /var/lib, and once you're using install_directory as a method of installing a canonical permissions set, it's not unlikely you'll need different directories to have different owners or even just different "other" visibility.

[Hi-Angel]

Well, since we are talking about amount of code, I usually implement that differently.

1. Here, there is no point in contaminating global visibility scope with a variable that will never be used later. So I always declare them inline:

   foreach d : ['/usr/share/foo', '/usr/share/bar']
       install_emptydir(d)
   endforeach

   You might ask "but what if there're multiple paths?". Well, then I re-format the inline declaration to span multiple lines. As a matter of fact, here's one of the usages of install_emptydir from the project at work, with the names changed:

   foreach dir : ['/var/lib/foo/',
                  '/var/lib/bar',
                  '/var/lib/buzz/msg.sock/'
                 ]
     install_emptydir(dir, install_mode: 'rwx------', install_tag: 'foo')
   endforeach

2. Similarly, if the paths are small I'd make them one line:

   install_emptydir('/usr/share/foo', '/usr/share/bar')

3. This one is same as yours

   install_emptydir('/usr/share/foo')
   install_emptydir('/usr/share/bar')

Option 3 takes fewer lines

As you can see, not necessarily 😊 That said, even if you break option 2 to 2 lines, it is still less to read than in option 3 simply because there's just one function call that does the same thing to all its args.

Imagine having 5 install_emptydirs as compared to just one with 5 paths inline. You'd need to read each call to make sure it is install_emptydir and not some other install_foo. Reading a single call with multiple args is faster.

Option 3 is also required if you need to specify different install_mode values, which I considered to be particularly likely at the time -- installing an empty directory is a very important use case when it's not even possible for the application to create the directory later on because it doesn't run as root but needs to use a dedicated application directory in /etc or /var/lib, and once you're using install_directory as a method of installing a canonical permissions set, it's not unlikely you'll need different directories to have different owners or even just different "other" visibility.

Well, in the project at work I have 3 calls to install_emptydir with rwx------ mode. Don't have a large variety of modes for empty dirs…

[Hi-Angel]

Ah, right, I was thinking of why not try applying your style from point 3 to the project at work and figured out: you see, an install_emptydir may have more than one arg. Usually it at least has a install_tag: 'some_name', similarly to all install_* functions in our project.

Now, if you were to copy the install_emptydir on each separate line as you suggest in point 3, you'd have to copy these additional args every time as well. This is why I have a foreach cycle instead, it makes it easier to read.

[tristan957]

I think that unless we agree to add this to tall the install functions, we should not merge this. I have no problem doing it to all of them, but this would be the only one if the PR as-is were to be merged.