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

A couple of improvements #22

Merged
merged 3 commits into from
Jan 5, 2023
Merged

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Dec 24, 2022

See the individual commits for why each change is an improvement in my opinion.

@l0kod
Copy link
Member

l0kod commented Jan 3, 2023

Thanks for these improvements!

My only concern is about the AsRawFd replacement with AsFd and OwnedFd which are not available with current Rust editions, but only starting with Rust 1.63. Could we conditionally use this type when it is available with supported Rust versions?

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jan 3, 2023

I will drop the AsFd change for now as switching between AsRawFd and AsFd is a breaking change, so can't be done depending on the rust version. For future reference this commit was 4a32263.

@l0kod
Copy link
Member

l0kod commented Jan 3, 2023

I will drop the AsFd change for now as switching between AsRawFd and AsFd is a breaking change, so can't be done depending on the rust version. For future reference this commit was 4a32263.

I'm improving this crate for a new major release, so I don't mind breaking the API now (especially this small), but it seems reasonable to be compatible with at least the latest Rust edition. I tested this commit and it passes all the tests without changing the Rust edition, so I guess it's not an issue (as long as rustc >= 1.63, which should be fine) and it will still be compatible with the Rust 2018 and 2021 editions, right?

@l0kod
Copy link
Member

l0kod commented Jan 3, 2023

Anyway, the cargo doc --workspace --all-features (not part of the CI yet) threw a warning with the AsFd commit.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jan 3, 2023

Nothing in this PR should cause any MSRV increase.

I'm improving this crate for a new major release, so I don't mind breaking the API now (especially this small)

I mean that if I use an AsFd bound when rustc >=1.63 is detected and AsRawFd otherwise, then the API will break between those cases, so you can have code which works on rustc 1.62, but breaks on rustc 1.63 due to rust-landlock automatically switching from AsRawFd to AsFd.

I tested this commit and it passes all the tests without changing the Rust edition, so I guess it's not an issue (as long as rustc >= 1.63, which should be fine) and it will still be compatible with the Rust 2018 and 2021 editions, right?

In rust editions allow breaking syntactic changes to be made in a way that allows older code to keep working on newer rustc versions by staying on an older edition. Anything that can be enabled on older editions will be enabled on them. So if a new api in libstd is introduced in a future rustc version, all editions will be allowed to use it when compiling with this future rustc version. As such there is no point in staying on a rust edition older than the latest edition supported by the oldest rustc version you want to use. For example if you want to support rustc 1.56 you will have to stay on the 2018 edition, but if you only support rustc 1.57 (which introduced the 2021 edition) then you can use the 2021 edition without having any effect on users of your crate (as editions are invisible outside of the crate itself) or users who want to compile a project that uses your crate (as you already only support rustc 1.57 in this example)

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jan 3, 2023

By the way what is actually the oldest rustc version you want to keep supporting? If you haven't decided yet, here are some suggestions:

  • 1.48 (rustc version shipped with debian 11, no landlock support)
  • 1.54 (newest version of rustc that the mrustc alternative rust compiler can directly build)
  • 1.61 (rustc version shipped with Ubuntu 22.04 LTS, has landlock v1 abi support)
  • 1.63 (rustc version currently shipped with the upcoming debian 12, has landlock v2 abi support)
  • 1.65 (previous rustc version)

@l0kod
Copy link
Member

l0kod commented Jan 4, 2023

Thanks for these explanations.

OK, so the Rust edition doesn't imply to stick with a specific Rust version except for related syntax breaking changes, which is not the case to support AsFd. The remaining question is to pick a minimal Rust version to build this crate. Because AsFd is an important safety feature (that wasn't available when I started this crate), I'd very much like to choose the 1.63 Rust version as the minimal one. Furthermore at least Debian Sid and Arch Linux have Rust >= 1.63, and it is really easy to pick a newer version with rustup.

Please create a new PR with the AsFd commit. You can also update the readme to specify the MSRV.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jan 4, 2023

Opened #25.

src/ruleset.rs Show resolved Hide resolved
This allows using anyhow::Error as error type even though it doesn't
implement std::error::Error.

Signed-off-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
PathEnvError is not matched on, so it has no benefit over only using
anyhow.

Signed-off-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
These haven't been necessary since the 2018 edition

Signed-off-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
@l0kod l0kod merged commit 5aa8d33 into landlock-lsm:main Jan 5, 2023
@bjorn3 bjorn3 deleted the misc_improvements branch January 5, 2023 14:19
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