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.

Sign up for GitHub

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

Jump to bottom

Add support for capabilities #94

Merged
merged 4 commits into from
Sep 20, 2023
Merged

Add support for capabilities #94

merged 4 commits into from
Sep 20, 2023

Conversation

dsteeley
Copy link
Contributor

@dsteeley dsteeley commented Sep 1, 2023 •
edited
Loading

Add support for setting capabilities on assets.

Exposing functionality added to rpm-rs in this PR, rpm-rs/rpm#166

@cat-in-136 cat-in-136 added the enhancement New feature or request label Sep 2, 2023
@cat-in-136
Copy link
Owner

The change is unexpectedly complexed and will take some time to review.

Capability-value is validated caps() in rpm::FileOptionsBuilder (which takes a string as an argument instead of capctl::FileCaps) unlike other paramters. So, this got a bigger change than I expected.

@dsteeley
Copy link
Contributor Author

dsteeley commented Sep 5, 2023

The change is unexpectedly complexed and will take some time to review.

Capability-value is validated caps() in rpm::FileOptionsBuilder (which takes a string as an argument instead of capctl::FileCaps) unlike other paramters. So, this got a bigger change than I expected.

Do you have any recommendations for simplifying the change here?

My understanding was that cargo-generate-rpm is a wrapper around rpm-rs and so I have exposed the caps field of rpm-rs without additional validation in cargo-generate-rpm.

cat-in-136
cat-in-136 requested changes Sep 7, 2023
View reviewed changes
Copy link
Owner

@cat-in-136 cat-in-136 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@dsteeley Could you please confirm my comment and change your code?

It took long time to review... Regarding to the rpm crate design, I don't think simplifying than your code structures is not possible. Anyway, I would like you to add an new appropriate error type as I written in my review comment.

src/config/file_info.rs Show resolved Hide resolved
src/config/file_info.rs Outdated Show resolved Hide resolved
src/config/file_info.rs Outdated Show resolved Hide resolved
@dsteeley
Copy link
Contributor Author

I have applied markups as per your comments, thank you for reviewing.

Regarding your initial comment about rpm-rs taking a string and validating. That matches the existing group, user and mode behaviour of rpm-rs.
I believe capctl provided slightly different behaviour in the scenario where a capability wasn't specified "=" vs " " and capctl is only used for validation rather than directly for reading and writing the RPM capability header.

See rpm-rs/rpm#166 (comment) for the discussion on usage of capctl vs parsing as a string

@cat-in-136 cat-in-136 merged commit ae2792b into cat-in-136:master Sep 20, 2023
1 check passed
@dsteeley dsteeley deleted the support_capabilities branch September 20, 2023 13:05
@ximon18 ximon18 mentioned this pull request Jan 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cat-in-136 cat-in-136 cat-in-136 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dsteeley @cat-in-136