Apply rules to implicitly created directories (Issue #85)

[moedan]

Bugfix to apply rules to implicitly created intermediate directories.

See issue #85 for details.

[ctron]

Sorry for the review, I lost track of this PR.

I noticed a bunch of test failing, and for good reason. I think the basic idea is valid. However, that outcome is that also directories like /etcget added. to my understanding that will let the RPM take ownership of the directory which might cause two issues:

1. There could be a conflict when another file claims that directory
2. The directory would get removed when the RPM is being uninstalled

The latter might be an issue in cases like /etc.

I am also not quite sure that the idea of ignoring by prefix works as intended. Adding a prefix of /etc would basically remove directories starting with /etc.

I am not sure what a better approach would be. Maybe only adding directories up the path that was requested for scanning. Meaning: if you as the plugin to start scanning /foor/bar/baz for adding to /usr/lib/app, then just add directories automatically starting with /usr/lib/app?

Another approach could be to add a prefix list explicitly enabling this feature for those prefixes.

[moedan]

Thank you for your feedback! I have thought about it a bit. I don't think there is a one-size-fits-all approach, which I tried to follow in my first implementation. That's why I've now chosen the approach where you explicitly specify a list of prefixes for which this feature is applied.

I've also added an integration test for this new feature.

[ctron]

I very much prefer this approach. I like the opt-in character of it. And I think it a reasonable concept to understand. Good idea!

I am not so sure about the naming. Naming is hard I know :) … And am not trying to push back on the current naming, just checking if we could come up with something better.

Basically it's a set of base directories we enable for automatic generation of intermediate directories, starting from the provided base, right?

[moedan]

Yes, naming isn't so easy. I'm open for suggestions. ;)

Basically it's a set of base directories we enable for automatic generation of intermediate directories, starting from the provided base, right?

Yes, you're right.

---

While browsing through the source code and documentation, I came across the directories flag from the collect entry:

  <entry>
    <name>/usr/lib</name>
    <collect>
      <from>src/main/resources/lib</from>
      <directories>true</directories> <!-- make explicit directories -->
    </collect>
  </entry>

If the flag is active, the intermediate folders are created explicitly (no problem with my approach). But if the flag is inactive, intermediate folders are still generated if the paths correspond to the base folders of my extension.
Do you think this is a problem or could cause confusion?

[ctron]

If the flag is active, the intermediate folders are created explicitly (no problem with my approach). But if the flag is inactive, intermediate folders are still generated if the paths correspond to the base folders of my extension.
Do you think this is a problem or could cause confusion?

Hm, … looks like I didn't work with that plugin for a while. So yes, that would be correct approach when "scanning"/"collecting".

I am wondering if this now boils down to a documentation issue: If you scan, we have a flag for this. If you put files explicitly, you need to create directories yourself.

I would somehow still feel your extension useful. But indeed maybe confusing. I am open for suggestions.

[ctron]

I am wondering, what the improvement with this PR over the current situation? I am not trying to push back, but coming up with a good description of it. Like an example use case where things are much better now.

[moedan]

Sorry for my late reply. Something came up after I pushed my commit.

---

I had a bad feeling about overwriting the field directories from collect entry configuraiton. So changed my code to not changing the behavior of collect entry itself.

Current behavior for following example:

• The intermediate directories (including the target) of name-field /opt/mycompany/myapp/a/b are added explicitly to rpm package
• The directories from inside of collect are added dependent on directories flag to rpm package

<generateIntermediateDirectories>
    <baseDirecotry>/opt/mycompany/myapp</baseDirecotry>
</generateIntermediateDirectories>

<entry>
    <name>/opt/mycompany/myapp/a/b</name>
    <collect>
        <from>${project.basedir}/src/main/data</from> <!-- contains only one file /x/y/foobar -->
        <directories>false</directories> <!-- make explicit directories -->
    </collect>
    <ruleset>my-default</ruleset>
</entry>

Result with directories=false:

/opt/mycompany/myapp
/opt/mycompany/myapp/a
/opt/mycompany/myapp/a/b
/opt/mycompany/myapp/a/b/x/y/foobar

Result with directories=true:

/opt/mycompany/myapp
/opt/mycompany/myapp/a
/opt/mycompany/myapp/a/b
/opt/mycompany/myapp/a/b/x
/opt/mycompany/myapp/a/b/x/y
/opt/mycompany/myapp/a/b/x/y/foobar

I don't know a real use case for this situation. Perhaps my extension should override the behavior of the dictionary property in collection entry after all (behavior before last commit). I'm open for your suggestions.

[ctron]

Thanks for the explanation. So I think it's better to keep this independent of the directories flag, as it seems those are actually two independent settings.

The directories setting actually defaults to true, so it should automatically create directories.

The change you added would allow one to control the automatic generation of directories outside of the collector, right? I think that's a good feature. We just need to document that properly :) Otherwise it might get a bit confusing. And I think it might help having your example as part of this. I also think the naming is fine when it comes to that.

I know you've done a lot already, and this drag on a bit. But I would hope that you could do a final thing, add some docs around here:

rpm-builder/src/site/markdown/entry.md

Lines 52 to 54 in 74ecb1a

	The collect elements requires one additional element: `<from>` which defines the base path. In addition
	there is the optional element `<directories>`, which can be set to `false` in order to not record
	directories explicitly. **Note:** the `<from>` directory itself will never be added as explicit directory. This can be done using an additional `<entry>` element.

Maybe create a small new sections about auto-creation of directories at the end of the file, but also mention the difference between both concepts in this location linked. It doesn't need to be much, just to explain the difference of both mechanisms. That would be awesome.

[ctron]

It's good in any case. Maybe we can add some quick docs.

[moedan]

Thanks for your positive feedback! I'll try to contribute the requested improvements to the documentation on Monday. Perhaps I will come up with a better name for the configuration parameter by then.

The change you added would allow one to control the automatic generation of directories outside of the collector, right?

Yes, that's correct.

[ctron]

That should be the next minor version (major.minor.patch).

[ctron]

This is awesome! Thanks.

The next version would be 1.11.0, if you could replace that, I'll merge and try to get it out today.

[moedan]

Thanks for your very fast response!

I have changed the version strings in documentation to 1.11.0.