task #54: Template Directories for Debian package #63
Conversation
| // Create files and directories | ||
| mappings foreach { | ||
| case LinuxPackageMapping(paths, perms, zipped) => | ||
| val (dirs, files) = paths.partition(_._1.isDirectory) |
There was a problem hiding this comment.
Nice. the nesting here is starting to make me want helper functions, but the logic looks excellent.
There was a problem hiding this comment.
Could refactor it, however the logic is so small and almost non redudant. It's a bit more verbose, but easier to read IMHO ;)
Took me a while to get your for loop construction, never used or saw it that way before.
There was a problem hiding this comment.
Hmm... Not what I want to hear.... I tend to like that mechanism of for construction :). In any case, the cleanups are nice.
|
Looks great! There's still one nagging issue (it's one I avoided for a bit): the executable bit for directories. I'll comment directly in the code. |
| files map { | ||
| case (file, name) => (file, t / name) | ||
| } foreach { | ||
| case (source, target) => copyAndFixPerms(source, target, perms, zipped) |
There was a problem hiding this comment.
So here's something I was being lazy with before and never really fixed, because I wanted the user interface for defining package mappings to have some flexibility and be somewhat lazy.
When we're chmoding things and globbing over files, we were bumping directories to have eveyrone readable + executable permissions (0755). When you glob over a set of files and create mappings (like LinuxPackageMappings(binDir.*** x relativeTo(rootDir)), ...), you're including files + directories with the same set of Permissions. However, directories need the appropriate executable bits set if you're to be able to list files inside them.
SOOOOOO...... What we may want when chmoding directories is to detect "read" permissions for user, group, world and automatically bump in executable permissions on directories.
Again I was too lazy to do this by default. There's also the odd scenario that someone may not want this. So option #2 would be to modify the external functions people use to set up linux package mappings to create two objects, one for directories (with auto-executable permissions) and one with files (without adding executable permissions).
I think I prefer plan A. What are your thoughts?
There was a problem hiding this comment.
Hm, I don't get the point. I ran a few tests with
linuxPackageMappings in Debian += packageTemplateMapping(target.value,
"/var/log/" + name.value,
"/etc/" + name.value
)
linuxPackageMappings in Debian += packageTemplateMapping(target.value,
"/var/log/private"
).withPerms("0644")
linuxPackageMappings in Debian += packageMapping(
(baseDirectory.value / "conf" / "application.conf") -> "/root/application.conf",
(baseDirectory.value / "conf" / "application.conf") -> ("/etc/" + name.value + "/application.conf")
).withPerms("0644")and all permissions were set as they should. If you mean that users may get confused with the directory permissions, then I think it's a matter of documentation. Less magic, is better magic IMHO :)
The LinuxFileMetaData have by default the execution bit so we only have to add magic (Plan A) if this behaviour changes. Right?
There was a problem hiding this comment.
Yeah, see requests like #69 . I think you're right. We should not do any magic in the "core" api, and we can push helper methods on top to simplify discovering files in directories for users. Your thoughts welcome there!
|
BTW - I plan to merge this regardless. If need, I'll push the executable fix on top of what you have. Great work. |
|
Again, thanks much for the work! |
task #54: Template Directories for Debian package
After a second round, I think I managed to get everything working.
This pullrequest could then be closed without merging.
DSL would look like