Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify file-related information under FileEntry (get_file_entries) #188

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

marxin
Copy link
Contributor

@marxin marxin commented Oct 24, 2023

Support all file-related functions for a case where an RPM is free of files.

@dralley
Copy link
Collaborator

dralley commented Oct 25, 2023

We have an "empty" rpm with no files here, I'd prefer if you used that instead of adding a new one

@dralley
Copy link
Collaborator

dralley commented Oct 25, 2023

Also, I would kind of prefer to delete these methods entirely, and just make sure their functionality is present via get_file_entries(). I don't see much point in getting a list of file checksums or signatures without having access to either the contents or at least the path of the file, and making the user join them back together manually wouldn't be great API.

@marxin marxin force-pushed the rpm-with-no-files branch from 17028c9 to ff07ba0 Compare October 26, 2023 06:04
@marxin
Copy link
Contributor Author

marxin commented Oct 26, 2023

We have an "empty" rpm with no files here, I'd prefer if you used that instead of adding a new one

Thanks, I've just done that.

@marxin
Copy link
Contributor Author

marxin commented Oct 26, 2023

I don't see much point in getting a list of file checksums or signatures without having access to either the contents or at least the path of the file, and making the user join them back together manually wouldn't be great API.

Yes, I also agree with the suggested approach. The only small drawback of the get_file_entries approach is that you need to parse many RPM Tag sections before you can assemble a FileEntry.

@dralley
Copy link
Collaborator

dralley commented Oct 26, 2023

Yes, I also agree with the suggested approach. The only small drawback of the get_file_entries approach is that you need to parse many RPM Tag sections before you can assemble a FileEntry.

What do you mean? The whole header is "parsed" up-front. This function does clone a few values, and the timestamps and digests have a little bit of parsing, but I don't think it's particularly expensive.

Ultimately it's not that difficult to fall back to fetching the tags directly if get_file_entries() isn't sufficient for some reason.

@marxin
Copy link
Contributor Author

marxin commented Oct 26, 2023

What do you mean? The whole header is "parsed" up-front.

Ah, ok, that's what I mean. So if it's parsed, then it's fine. If you want, I can prepare yet another pull request where I'll unify all under the umbrella of get_file_entries function.

@marxin
Copy link
Contributor Author

marxin commented Oct 26, 2023

The question is if you want to do it instead of this PR, or after that. What do you prefer?

@dralley
Copy link
Collaborator

dralley commented Oct 26, 2023

You can use this PR

@dralley
Copy link
Collaborator

dralley commented Oct 26, 2023

note to self: I really ought to finish my own PR that will allow fetching the contents of file entries out of the archive

@marxin
Copy link
Contributor Author

marxin commented Oct 26, 2023

Do you want to omit the get_file_paths function from the public API?

@dralley
Copy link
Collaborator

dralley commented Oct 26, 2023

That one I think could potentially be useful on its own (and isn't trivial to implement) so it should probably stay.

@marxin marxin force-pushed the rpm-with-no-files branch from ff07ba0 to 5995e14 Compare October 27, 2023 07:35
@marxin marxin changed the title Fully support RPM files without any file Unify file-related information under FileEntry (get_file_entries) Oct 27, 2023
Support all file-related functions for a case where an RPM
is free of files.
@marxin marxin force-pushed the rpm-with-no-files branch from 5995e14 to bc5a7f8 Compare October 27, 2023 12:57
Copy link
Collaborator

@dralley dralley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks!

@dralley dralley merged commit f677ffe into rpm-rs:master Oct 27, 2023
11 checks passed
@marxin marxin deleted the rpm-with-no-files branch October 27, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants