search

cavaliergopher / rpm

A Go implementation of the RPM file format
BSD 3-Clause "New" or "Revised" License
169 stars 44 forks source link

Add Modes() for getting file modes of the RPM contents #3

Closed andrewkroh closed 7 years ago

andrewkroh commented 7 years ago

This PR adds a Modes() method to PackageFile that returns the file mode of each file in the package.

cavaliercoder commented 7 years ago

Hey this is a great submission, thanks very much. Great tests too. I'd really like to accept this PR except that it conflicts with a planned change. This is, of course, open to your feedback.

My plan was to change packagefile.Files so that it returns a slice of structs that implement os.FileInfo, which would of course include FileInfo.Mode(). This way, the caller doesn't need to call packagefile.Files and correlate the returned slice against a call to packagefile.Modes().

What are your thoughts? Would you be interested in implementing this? Otherwise I can prioritise this for me to do.

cavaliercoder commented 7 years ago

Curiosity got the better of me and I implemented []FileMode. Please take a look and lend your thoughts. I was sure to include your PR in the commit history.

andrewkroh commented 7 years ago

The change you made you is what I originally had in mind, but I didn't want to make a breaking change to the API without checking first. So I'm quite happy to see you went ahead and implemented it. Thanks!

I'd like to have the UID and GID in the FileInfo too. So I may open a PR for that (unless you beat me to it).

cavaliercoder commented 7 years ago

Yeah, I had a moment of deliberation about the breaking change but figured I don't really have the scale of adoption for it to be an issue. Also... adding fuel to the fire for Go to implement some form of package versioning.

Please, go ahead and submit a PR for UID and GID. I presume these are complementary to owner/group?

andrewkroh commented 7 years ago

As it turns out RPM (ver >= 2) does not internally use UID/GID. At install time if the owner/group strings don't map to a UID/GID it just uses root for that file. I guess this make the packages more portable since they don't rely on having the same UID to username mapping everywhere.

cavaliercoder commented 7 years ago

Interesting. Perhaps we should just only return root for <v2?

andrewkroh commented 7 years ago

I think the current behavior is correct since go-rpm ignores the RPMTAG_FILEUIDS and RPMTAG_FILEGIDS. So no change is needed.

This was the tool that hinted to me that I should not be trying to read the RPMTAG_FILEUIDS values.

cavaliercoder commented 7 years ago

That makes sense. Great insight, thanks for posting it.