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

profile: Added comments and a finally functioning profile loader. #4

Closed
wants to merge 1 commit into from

Conversation

vdagonneau
Copy link
Contributor

This first version is really basic. It just loads a profile based on the
command name located in ~/.config/file_.ll.

The file format is also very basic, it is essentially just like dotenv
files: a succession of environment variable declaration. The only thing
"smart" about it for now is that it merges them before passing them to
the sandboxer.

Note: I included code in the comments much like for a normal crate.
However, it seems like code in the comments of examples are not
currently being executed anywhere: rust-lang/cargo#4508

This first version is really basic. It just loads a profile based on the
command name located in ~/.config/file_<cmd>.ll.

The file format is also very basic, it is essentially just like dotenv
files: a succession of environment variable declaration. The only thing
"smart" about it for now is that it merges them before passing them to
the sandboxer.

Note: I included code in the comments much like for a normal crate.
However, it seems like code in the comments of examples are not
currently being executed anywhere: rust-lang/cargo#4508
Copy link
Member

@l0kod l0kod left a comment

Choose a reason for hiding this comment

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

I think profile definitions deserve a proper configuration file format. We also need parsing and functional tests, as unit tests for functions but also for full profiles. This applies to the crate and this example.

/// ```
/// let paths = parse_path_list("/usr/lib:/bin:/tmp");
///
/// assert_eq!(paths, Ok("", vec!["/usr/lib", "/bin", "/tmp"]));
Copy link
Member

Choose a reason for hiding this comment

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

If it is not taken into account by cargo test then it should be in tests. Most of functions from this file should be tested.


let profile = parse_profile(&profile);
// TODO: no more hardcoding
let mut sandboxer = Command::new("./target/release/examples/sandboxer");
Copy link
Member

Choose a reason for hiding this comment

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

Why not reuse the code from the sandboxer as a library? In fact, it would avoid creating a custom dotenv-like parser. This could be replaced with a proper configuration format (e.g. TOML) that could also simplify the code and enable a more extensible configuration.

I'm working on a version of add_rule() that takes a Rule iterator as argument. This would help simplify and reuse the sandboxer code.

@l0kod
Copy link
Member

l0kod commented Aug 5, 2022

This new application should probably be hosted in a dedicated repository. I'll probably keep the sandboxer example as an example that can be used to help users, but let's focus on the original goal: a library. :)

@l0kod l0kod closed this Aug 5, 2022
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.

None yet

2 participants