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

Structify more of the internals to simplify the implementation. #911

Merged
merged 3 commits into from
Jul 6, 2022

Conversation

Alexhuszagh
Copy link
Contributor

@Alexhuszagh Alexhuszagh commented Jul 5, 2022

Reduces the number of arguments to the function signatures, making the code much easier to understand, and relocates some things. For example, docker_in_docker is now associated with Engine, and now with cli::Args. Likewise, unstable doctests is now a part of Environment and not cli::Args. MessageInfo now stores mutable state (needs to clear lines) and is no longer copyable, and a single instance is created in lib and passed around everywhere as a mutable reference.

The changes with MessageInfo will allow us to erase lines in the future, both from stderr and stdout, which we currently don't use but will allow us to close #859.

This also fixes an issue where -vv+ was not recognized as a valid verbose flag: cargo can handle 1 or more v characters. Also allowed parsing of --color=value args, when we previously only accepted --color value.

Reduces the number of arguments to the function signatures, making the
code much easier to understand, and relocates some things. For example,
`docker_in_docker` is now associated with `Engine`, and now with
`cli::Args`.
 Likewise, unstable doctests is now a part of `Environment` and not
 `cli::Args`. `MessageInfo` now stores mutable state (needs to clear
 lines) and is no longer copyable, and a single instance is created in
 lib and passed around everywhere as a mutable reference.

 This also fixes an issue where `-vv+` was not recognized as a valid
 verbose flag: `cargo` can handle 1 or more `v` characters. Also allowed
 parsing of `--color=value` args, when we previously only accepted
 `--color value`.
@Alexhuszagh Alexhuszagh added meta issues/PRs related to the maintenance of the crate. no changelog A valid PR without changelog (no-changelog) labels Jul 5, 2022
@Alexhuszagh
Copy link
Contributor Author

Still a draft since many things changed and I need to triple check no bugs were introduced. This refactors a lot of things to try to simplify our logic internally. The only actual changes in terms of behavior for the user are bug fixes:

  1. --color=value is now valid
  2. -vvvvvvvvvvvv or as many arguments of v are now valid

Both are consistent with cargo.

@@ -149,6 +149,18 @@ pub enum Volumes {
Remove(RemoveVolume),
}

macro_rules! volumes_get_field {
($self:ident, $field:ident $(.$cb:ident)?) => {{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid a lot of redundant code, I added macros so we can extract a single field from an enumeration. The callback allows to add as_deref if needed.

}
}
}
fn as_verbosity<T, C: Fn(&mut MessageInfo) -> T>(&mut self, call: C, new: Verbosity) -> T {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added these functions so we can temporarily change the verbosity of a function call, either to silence or to be more verbose, etc. It allows us to store the old value, and return a result and ensure the old state is restored.

message!(@status stream, $status, $message, $color, $msg_info)
}};
}
/// prints a red 'error' message and terminates.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also moved away from free functions since we store state now.


macro_rules! get_engine {
($args:ident, $docker_in_docker:expr, $msg_info: ident) => {{
get_container_engine($args.engine(), $docker_in_docker, &mut $msg_info)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These macros just simplify a lot of redundant code that existed earlier, while being relatively transparent.

}
}
Ok(())
}

fn is_verbose(arg: &str) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This corrects the behavior so -vvvvv and similar are accepted as arguments to get the verbosity. Previously this was passed to cargo, but not correctly processed for cross itself.

Copy link
Member

Choose a reason for hiding this comment

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

Nice, was meaning to fix this myself

}
}

enum ArgKind {
Copy link
Contributor Author

@Alexhuszagh Alexhuszagh Jul 5, 2022

Choose a reason for hiding this comment

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

Allows us to determine if the argument is the field or starts with the field in a single check, and then we can choose to process it immediately after.

@@ -167,37 +223,17 @@ pub fn parse(target_list: &TargetList) -> Result<Args> {
}
}

let msg_info = shell::MessageInfo::create(verbose, quiet, color.as_deref())?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should have a single instance, so it's been moved to lib.

@@ -167,37 +223,17 @@ pub fn parse(target_list: &TargetList) -> Result<Args> {
}
}

let msg_info = shell::MessageInfo::create(verbose, quiet, color.as_deref())?;
let docker_in_docker = if let Ok(value) = env::var("CROSS_CONTAINER_IN_CONTAINER") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a CLI argument, so it's been moved to Engine.

false
};

let enable_doctests = env::var("CROSS_UNSTABLE_ENABLE_DOCTESTS")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a CLI argument, so this has been moved to config.

.ok()
}

fn custom_toolchain(&self) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better we don't process all our config options for environment variables in lib itself, since variables like this are part of the configuration.

@@ -22,27 +22,38 @@ pub enum EngineType {
pub struct Engine {
pub kind: EngineType,
pub path: PathBuf,
pub in_docker: bool,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

docker_in_docker is natural here, since it's part of the behavior of the engine. We also only need it to determine the mount finder, so it's a good place.

@Emilgardis
Copy link
Member

Great! looks like a straightforward interface improvement

pub(crate) fn run(
engine: &Engine,
target: &Target,
options: DockerOptions,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restructuring into 2 logical structs simplifies our function signatures a lot, especially in docker::run and the like.

.wrap_err("when copying seccomp profile")?;
docker_user_id(&mut docker, engine.kind);
docker_seccomp(
&mut docker,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried not to overly rely on just having the structs passed: if there's any question of if the logic could need to be accessed outside and the logic does not rely on most of the members or it would require a lot of work to create those members, I've avoided passing the struct itself.

struct DeleteVolume<'a>(&'a Engine, &'a VolumeId, MessageInfo);
// it's unlikely that we ever need to erase a line in the destructors,
// and it's better than keep global state everywhere, or keeping a ref
// cell which could have already deleted a line
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The deleters are the only location we don't have a single instance of MessageInfo, but I think this is fine since it avoids having to pass a RefCell everywhere, worrying about avoiding multiple mutable borrows, etc., and the chance we delete a line when invoking the deleters is near 0.

fn print(&self, msg_info: MessageInfo) -> Result<()> {
shell::print(&self.fmt_message(msg_info), msg_info)
fn print(&self, msg_info: &mut MessageInfo) -> Result<()> {
let msg = self.fmt_message(msg_info);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be in 2 steps to avoid multiple mutable borrows.

@@ -338,7 +338,7 @@ impl From<Host> for Target {
Host::Aarch64UnknownLinuxMusl => Target::new_built_in("aarch64-unknown-linux-musl"),
Host::Other(s) => Target::from(
s.as_str(),
&rustc::target_list(Verbosity::Quiet.into()).unwrap(),
&rustc::target_list(&mut Verbosity::Quiet.into()).unwrap(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once again, we will never need to delete a line from the output here, so this isn't an issue.

@Alexhuszagh Alexhuszagh marked this pull request as ready for review July 6, 2022 00:08
@Alexhuszagh Alexhuszagh requested a review from a team as a code owner July 6, 2022 00:08
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 6, 2022

Build succeeded:

@bors bors bot merged commit 3dc0d1f into cross-rs:main Jul 6, 2022
@Alexhuszagh Alexhuszagh deleted the structify branch July 7, 2022 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta issues/PRs related to the maintenance of the crate. no changelog A valid PR without changelog (no-changelog)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants