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

caps: change API to borrow, update to 2018 edition #42

Closed
wants to merge 7 commits into from

Conversation

dvc94ch
Copy link

@dvc94ch dvc94ch commented Sep 16, 2019

  • Adds an example, and changes api to borrow CapsHashSet.
  • Fixes some warnings
  • Upadated for 2018 edition

@dvc94ch
Copy link
Author

dvc94ch commented Sep 21, 2019

@lucab: if you're not set strongly on using a pre-historic rust version I think this PR is pretty uncontroversial :)

@lucab
Copy link
Owner

lucab commented Sep 23, 2019

@dvc94ch thanks for the PR! I'll try to find a free slot of time this week to go through and review it. Updating the minimum version to something reasonable is not a problem.

Copy link
Owner

@lucab lucab left a comment

Choose a reason for hiding this comment

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

Patches looks very good (you could have probably split them in multiple PRs, but thanks anyway).
My only major concern is the switch to failure, which I'd rather not do at this point.

@@ -1,8 +1,7 @@
language: rust

rust:
- nightly-2018-07-24 # pinned toolchain for clippy
- 1.26.0 # minimum supported toolchain
- nightly-2019-09-13 # pinned toolchain for clippy
Copy link
Owner

Choose a reason for hiding this comment

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

You can pin this to 1.38.0 now that clippy is available on the stable channel.

@@ -1,8 +1,7 @@
language: rust

rust:
- nightly-2018-07-24 # pinned toolchain for clippy
- 1.26.0 # minimum supported toolchain
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of dropping this, it would be better to update to a recent stable. If we can still support 1.31.0 I'd be happier, but otherwise 1.37.0 would be fine.

@@ -14,7 +15,7 @@ exclude = [

[dependencies]
errno = "0.2"
error-chain = {version = "0.12", default-features = false}
failure = "0.1"
Copy link
Owner

Choose a reason for hiding this comment

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

While I acknowledge that error_chain is dead, I'd rather not do this for two reasons:

  • failure is dead too
  • this introduces a dependency on a C library via backtrace

For the future, I'm keeping an eye on snafu but I'd like to see this fixed first: shepmaster/snafu#113.

Copy link
Author

Choose a reason for hiding this comment

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

Interesting. The problem with error_chain I encountered (if I remember correctly) was that the errors where not Send.

Copy link
Owner

Choose a reason for hiding this comment

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

Indeed, that's painful if your consumer is on failure as different decisions were taken between the two libraries. You'll need to re-wrap the error at the border, for the moment.

println!("Permitted {:?}", perm);
let eff = caps::read(None, CapSet::Effective).unwrap();
println!("Effective {:?}", eff);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Do you maybe want to add a couple of assert_eq here at the bottom?

@lucab lucab changed the title misc caps: change API to borrow, update to 2018 edition Oct 2, 2019
@dvc94ch
Copy link
Author

dvc94ch commented Jan 16, 2020

closing for now

@dvc94ch dvc94ch closed this Jan 16, 2020
@lucab
Copy link
Owner

lucab commented Jan 16, 2020

@dvc94ch I started splitting out some of the changes from here to master.
If you are still interested, there are two things still missing from here:

  • dropping error_chain (moving to thiserror or snafu will do)
  • change to borrow input parameters

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