Allow Cargo.toml as configuration source
#754
Conversation
There was a problem hiding this comment.
Excellent work, could you potentially quash the commits into 1 or 2, and then implement the above changes? I'm in favor, if any other reviewer approves, I'd happily merge this.
Specificially, these two commits sound great:
- Add parsing of CrossToml from Cargo.toml
- Add documentation about config in Cargo.toml
So one commit would just be the source files, and the other would just be the documentation, which should make it easy?
Basically, just the following:
# reset to main, add the source files, and do the initial commit
git reset --soft main
git add src CHANGELOG.md
git commit -m "Add parsing of CrossToml from Cargo.toml"
git add docs README.md
git commit -m "Add documentation about cargo metadata config"
Please don't deprecate |
|
That's a very good point @Jules-Bertholet, thanks for raising it. This makes me wonder, maybe it does make sense to allow configuration in both files, and |
|
Considering what @Jules-Bertholet said, I agree with @Emilgardis that |
@otavio Did you mean "I agree with @Emilgardis that Cross.toml should have higher priority" ? |
|
@Jules-Bertholet yes. Sorry for the confusion. I edited my comment. |
|
Thx, I squashed the comments (accidentally into one instead of two) and made the requested changes, including giving a slightly better overview of the configuration options. |
Thanks, I'm going to go ahead and give your changes as patches to main (which I'll attach here) so you can easily fix the merge conflicts by hard resetting to main and then This could be done by: match (
cross_config_path.exists(),
cargo_cross_toml_opt.map(|(cfg, _)| cfg),
) {
(true, _) => {
// use cross.toml ....
Ok(Some(config))
}
(false, Some(cfg)) => Ok(Some(cfg)),
(false, None) => {
// Checks if there is a lowercase version of this file
if root.path().join("cross.toml").exists() {
eprintln!("There's a file named cross.toml, instead of Cross.toml. You may want to rename it, or it won't be considered.");
}
Ok(None)
}
}This is just a slightly logic change from before. |
|
In addition, I think the better solution is some kind of merge, e.g consider both files, but any config set in |
|
I don't think it should be solved with a bunch of function changes in the config, rather use serde_json and extend/push to a |
|
Good idea. Anyway, here's the entire patch to fix the merge conflicts relative to the current main, or commit 23fd0af. Just save it to a file, reset hard to main, and then apply it. We should definitely make the changes Emil mentioned though prior to merging. diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2156124..4042929 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- #775 - forward Cargo exit code to host
- #767 - added the `cross-util` and `cross-dev` commands.
+- #754 - Add `Cargo.toml` as configuration source
- #745 - added `thumbv7neon-*` targets.
- #741 - added `armv7-unknown-linux-gnueabi` and `armv7-unknown-linux-musleabi` targets.
- #721 - add support for running doctests on nightly if `CROSS_UNSTABLE_ENABLE_DOCTESTS=true`.
diff --git a/README.md b/README.md
index 763265f..d6871b0 100644
--- a/README.md
+++ b/README.md
@@ -82,9 +82,27 @@ $ cross rustc --target powerpc-unknown-linux-gnu --release -- -C lto
## Configuration
-You can place a `Cross.toml` file in the root of your Cargo project or use a
-`CROSS_CONFIG` environment variable to tweak `cross`'s behavior. The format
-of `Cross.toml` is documented in [docs/cross_toml.md](docs/cross_toml.md).
+You have three options to configure `cross`. All of these options use the TOML format for configuration and the possible configuration values are documented [here](docs/cross_toml.md).
+
+### Option 1: Configuring `cross` directly in your `Cargo.toml`
+
+You can directly set [configuration values](docs/cross_toml.md) in your `Cargo.toml` file, under the `[package.metadata.cross]` table, i.e. key prefix.
+An example config snippet would look like this:
+
+```
+[package.metadata.cross.target.aarch64-unknown-linux-gnu]
+xargo = false
+image = "test-image"
+runner = "custom-runner"
+```
+
+### Option 2: Configuring `cross` via a `Cross.toml` file
+
+You can put your [configuration](docs/cross_toml.md) inside a `Cross.toml` file in your project root directory.
+
+### Option 3: Using `CROSS_CONFIG` to specify the location of your configuration
+
+By setting the `CROSS_CONFIG` environment variable, you can tell `cross` where it should search for the config file. This way you are not limited to a `Cross.toml` file in the project root.
### Custom Docker images
diff --git a/docs/cross_toml.md b/docs/cross_toml.md
index 662c541..58d390d 100644
--- a/docs/cross_toml.md
+++ b/docs/cross_toml.md
@@ -1,4 +1,6 @@
-The `cross` configuration in the `Cross.toml` file, can contain the following elements:
+The `cross` configuration in the `Cross.toml` file, can contain the elements described below.
+
+If the configuration is given in the `Cargo.toml`, these table headers must be of the form `[package.metadata.cross.<KEY>]`.
# `build`
The `build` key allows you to set global variables, e.g.:
@@ -38,4 +40,4 @@ This is similar to `build.env`, but allows you to be more specific per target.
[target.x86_64-unknown-linux-gnu.env]
volumes = ["VOL1_ARG", "VOL2_ARG"]
passthrough = ["IMPORTANT_ENV_VARIABLES"]
-```
\ No newline at end of file
+```
diff --git a/src/cross_toml.rs b/src/cross_toml.rs
index fbddb93..f20c6a6 100644
--- a/src/cross_toml.rs
+++ b/src/cross_toml.rs
@@ -2,7 +2,7 @@
use crate::errors::*;
use crate::{Target, TargetList};
-use serde::Deserialize;
+use serde::{Deserialize, Deserializer};
use std::collections::{BTreeSet, HashMap};
/// Environment configuration
@@ -46,11 +46,33 @@ pub struct CrossToml {
impl CrossToml {
/// Parses the [`CrossToml`] from a string
pub fn parse(toml_str: &str) -> Result<(Self, BTreeSet<String>)> {
- let tomld = &mut toml::Deserializer::new(toml_str);
+ let mut tomld = toml::Deserializer::new(toml_str);
+ Self::parse_from_deserializer(&mut tomld)
+ }
- let mut unused = BTreeSet::new();
+ /// Parses the [`CrossToml`] from a string containing the Cargo.toml contents
+ pub fn parse_from_cargo(cargo_toml_str: &str) -> Result<Option<(Self, BTreeSet<String>)>> {
+ let cargo_toml: toml::Value = toml::from_str(cargo_toml_str)?;
+ let cross_metadata_opt = cargo_toml
+ .get("package")
+ .and_then(|p| p.get("metadata"))
+ .and_then(|m| m.get("cross"));
+
+ if let Some(cross_meta) = cross_metadata_opt {
+ Ok(Some(Self::parse_from_deserializer(cross_meta.clone())?))
+ } else {
+ Ok(None)
+ }
+ }
- let cfg = serde_ignored::deserialize(tomld, |path| {
+ /// Parses the [`CrossToml`] from a [`Deserializer`]
+ fn parse_from_deserializer<'de, D>(deserializer: D) -> Result<(Self, BTreeSet<String>)>
+ where
+ D: Deserializer<'de>,
+ D::Error: Send + Sync + 'static,
+ {
+ let mut unused = BTreeSet::new();
+ let cfg = serde_ignored::deserialize(deserializer, |path| {
unused.insert(path.to_string());
})?;
@@ -204,4 +226,56 @@ mod tests {
Ok(())
}
+
+ #[test]
+ pub fn parse_from_empty_cargo_toml() -> Result<()> {
+ let test_str = r#"
+ [package]
+ name = "cargo_toml_test_package"
+ version = "0.1.0"
+
+ [dependencies]
+ cross = "1.2.3"
+ "#;
+
+ let res = CrossToml::parse_from_cargo(test_str)?;
+ assert!(res.is_none());
+
+ Ok(())
+ }
+
+ #[test]
+ pub fn parse_from_cargo_toml() -> Result<()> {
+ let cfg = CrossToml {
+ targets: HashMap::new(),
+ build: CrossBuildConfig {
+ env: CrossEnvConfig {
+ passthrough: vec![],
+ volumes: vec![],
+ },
+ xargo: Some(true),
+ default_target: None,
+ },
+ };
+
+ let test_str = r#"
+ [package]
+ name = "cargo_toml_test_package"
+ version = "0.1.0"
+
+ [dependencies]
+ cross = "1.2.3"
+
+ [package.metadata.cross.build]
+ xargo = true
+ "#;
+
+ if let Some((parsed_cfg, _unused)) = CrossToml::parse_from_cargo(test_str)? {
+ assert_eq!(parsed_cfg, cfg);
+ } else {
+ panic!("Parsing result is None");
+ }
+
+ Ok(())
+ }
}
diff --git a/src/lib.rs b/src/lib.rs
index c4c9808..4261926 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -143,7 +143,7 @@ impl<'a> From<&'a str> for Host {
}
#[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize)]
-#[serde(from = "&str")]
+#[serde(from = "String")]
pub enum Target {
BuiltIn { triple: String },
Custom { triple: String },
@@ -269,9 +269,9 @@ impl From<Host> for Target {
}
}
-impl From<&str> for Target {
- fn from(target_str: &str) -> Target {
- let target_host: Host = target_str.into();
+impl From<String> for Target {
+ fn from(target_str: String) -> Target {
+ let target_host: Host = target_str.as_str().into();
target_host.into()
}
}
@@ -500,27 +500,50 @@ pub(crate) fn warn_host_version_mismatch(
Ok(VersionMatch::Same)
}
-/// Parses the `Cross.toml` at the root of the Cargo project or from the
-/// `CROSS_CONFIG` environment variable (if any exist in either location).
+/// Obtains the [`CrossToml`] from one of the possible locations
+///
+/// These locations are checked in the following order:
+/// 1. Package metadata in the Cargo.toml
+/// 2. If the `CROSS_CONFIG` variable is set, it tries to read the config from its value
+/// 3. Otherwise, the `Cross.toml` in the project root is set.
fn toml(metadata: &CargoMetadata) -> Result<Option<CrossToml>> {
- let path = match env::var("CROSS_CONFIG") {
+ let root = &metadata.workspace_root;
+ let cross_config_path = match env::var("CROSS_CONFIG") {
Ok(var) => PathBuf::from(var),
- Err(_) => metadata.workspace_root.join("Cross.toml"),
+ Err(_) => root.join("Cross.toml"),
};
- if path.exists() {
- let content = file::read(&path)
- .wrap_err_with(|| format!("could not read file `{}`", path.display()))?;
-
- let (config, _) = CrossToml::parse(&content)
- .wrap_err_with(|| format!("failed to parse file `{}` as TOML", path.display()))?;
+ // Attempts to read the cross config from the Cargo.toml
+ let cargo_toml_str =
+ file::read(root.join("Cargo.toml")).wrap_err("failed to read Cargo.toml")?;
+ let cargo_cross_toml_opt = CrossToml::parse_from_cargo(&cargo_toml_str)?;
+
+ match (
+ cross_config_path.exists(),
+ cargo_cross_toml_opt.map(|(cfg, _)| cfg),
+ ) {
+ (true, _) => {
+ let content = file::read(&cross_config_path).wrap_err_with(|| {
+ format!("could not read file `{}`", cross_config_path.display())
+ })?;
+
+ let (config, _) = CrossToml::parse(&content).wrap_err_with(|| {
+ format!(
+ "failed to parse file `{}` as TOML",
+ cross_config_path.display()
+ )
+ })?;
+
+ Ok(Some(config))
+ }
+ (false, Some(cfg)) => Ok(Some(cfg)),
+ (false, None) => {
+ // Checks if there is a lowercase version of this file
+ if root.join("cross.toml").exists() {
+ eprintln!("There's a file named cross.toml, instead of Cross.toml. You may want to rename it, or it won't be considered.");
+ }
- Ok(Some(config))
- } else {
- // Checks if there is a lowercase version of this file
- if metadata.workspace_root.join("cross.toml").exists() {
- eprintln!("There's a file named cross.toml, instead of Cross.toml. You may want to rename it, or it won't be considered.");
+ Ok(None)
}
- Ok(None)
}
} |
|
I just fixed the merge conflict and would now try implementing the merge mechanism as proposed by @Emilgardis. |
Seems you've created a bunch of new commits :/ I can fix it for you on the branch if you want |
|
Here's how to fix it easily This will consolidate your changes and make it into one unstaged change. Then you can commit it as usual and push --force |
|
If you can fix the clippy lint and squash the commits, that'd be great. I'll also give it another review shortly. |
There was a problem hiding this comment.
Almost there, thanks for all the excellent work on this PR, I just think we should have the following:
First, change the doc comment here to:
/// Obtains the [`CrossToml`] from one of the possible locations
///
/// These locations are checked in the following order:
/// 1. If the `CROSS_CONFIG` variable is set, it tries to read the config from its value
/// 2. Otherwise, the `Cross.toml` in the project root is set.
/// 3. Package metadata in the `Cargo.toml`
///
/// The values from `CROSS_CONFIG` or `Cross.toml` are concatenated with the package
/// metadata in `Cargo.toml`. with `Cross.toml` having the highest priority.
Add code to merge the two. If you'd like, I can implement this, or feel free to as well. Just let me know.
|
I added a mechanism for merging both config sources, and addressed your requested changes, @Alexhuszagh. The
|
src/cross_toml.rs
Outdated
| /// The `build` fields ([`CrossBuildConfig`]) are merged based on their sub-fields. | ||
| /// A field in the [`CrossBuildConfig`] will only overwrite another if it contains | ||
| /// a value, i.e. it is not `None`. | ||
| pub fn merge(&self, other: &CrossToml) -> Result<CrossToml> { |
There was a problem hiding this comment.
Couldn't this be implemented before deserializing the two/three data sources?
e.g take the file contents, make toml::Tables, extend them, and then deserialize back.
If we serialize like this we lose unused keys, losing some information when we report back, ofc, this isn't a problem because we reported it already when making
src/cross_toml.rs
Outdated
| /// Parses the [`CrossToml`] from a string containing the Cargo.toml contents | ||
| pub fn parse_from_cargo(cargo_toml_str: &str) -> Result<Option<(Self, BTreeSet<String>)>> { | ||
| let cargo_toml: toml::Value = toml::from_str(cargo_toml_str)?; | ||
| let cross_metadata_opt = cargo_toml | ||
| .get("package") | ||
| .and_then(|p| p.get("metadata")) | ||
| .and_then(|m| m.get("cross")); | ||
|
|
||
| if let Some(cross_meta) = cross_metadata_opt { | ||
| Ok(Some(Self::parse_from_deserializer(cross_meta.clone())?)) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| } |
There was a problem hiding this comment.
this is wrong, it should use crate::cargo::cargo_metadata_with_args(Some(cwd), None, false)
There was a problem hiding this comment.
discard that thought, it does already, my bad!
There was a problem hiding this comment.
Good one, I wasn't aware of that.
There was a problem hiding this comment.
Unresolving this, you can actually use this fact for the better, instead of taking a string, add a package.metadata.cross: toml::Map to the CargoMetadata, not needed but it might help and you'll get it for free
|
Just improved the merging mechanism and made it more general. I'll check out what to extend in the current |
|
good job so far :D |
|
|
|
Ok I've submitted 2 different branches for how to handle the merging logic, one is for overwriting array values and the other is for merging them, and both do not assume the structure of the TOML file (which is an advantage, IMO): This is my sample [package.metadata.cross.target.target2.env]
passthrough = ["VAR2"]
volumes = ["VOL2_ARG"]And this is my sample [target.target2.env]
passthrough = ["VAR2_PRECEDENCE"]
volumes = ["VOL2_ARG_PRECEDENCE"]cargo_toml_merge_arraysIn this, two config files' arrays merge, which may not be desirable because we then cannot remove a value from an array, say, in [target.target2.env]
passthrough = ["VAR2", "VAR2_PRECEDENCE"]
volumes = ["VOL2_ARG", "VOL2_ARG_PRECEDENCE"]cargo_toml_overwrite_arraysThis would overwrite one array with another. The main difference here relative to the initial version is it does a recursive merge, which allows us to generalize about our structure (IE, not make assumptions about where optional values may or may not be present). This would produce something equivalent to: [target.target2.env]
passthrough = ["VAR2_PRECEDENCE"]
volumes = ["VOL2_ARG_PRECEDENCE"] |
Allows us to differentiate between an empty array and an array that was not provided when merging config files. cross-rs#754 (comment) Required for cross-rs#754.
Allows us to differentiate between an empty array and an array that was not provided when merging config files. cross-rs#754 (comment) Required for cross-rs#754.
Allows us to differentiate between an empty array and an array that was not provided when merging config files. cross-rs#754 (comment) Required for cross-rs#754.
Allows us to differentiate between an empty array and an array that was not provided when merging config files. cross-rs#754 (comment) Required for cross-rs#754.
801: Make config vectors optional. r=Emilgardis a=Alexhuszagh Allows us to differentiate between an empty array and an array that was not provided when merging config files. #754 (comment) Required for #754. Co-authored-by: Alex Huszagh <ahuszagh@gmail.com>
|
Now that #801 has been merged, merging the two config files and properly overwriting arrays is now possible. So the strategy I think we've agreed on is:
That is, given the two sample files: This is my sample [package.metadata.cross.build]
xargo = true
[package.metadata.cross.build.env]
passthrough = ["VAR1"]
[package.metadata.cross.target.target2]
build-std = true
[package.metadata.cross.target.target2.env]
passthrough = ["VAR2"]
volumes = ["VOL2_ARG"]And this is my sample [build.env]
volumes = ["VOL1_ARG"]
[target.target2]
image = "image2:tag"
[target.target2.env]
passthrough = ["VAR2_PRECEDENCE"]
volumes = ["VOL2_ARG_PRECEDENCE"]The output should be: [build]
xargo = true
[build.env]
passthrough = ["VAR1"]
volumes = ["VOL1_ARG"]
[target.target2]
build-std = true
image = "image2:tag"
[target.target2.env]
passthrough = ["VAR2_PRECEDENCE"]
volumes = ["VOL2_ARG_PRECEDENCE"] |
|
I like the recursive merging a lot, @Alexhuszagh, nice solution! I think merging the |
I think we need consistency, or else things get convoluted pretty quickly. And we need a way to be able to "unset" variables provided in Also, @mntns, you have my permission to use any code I write for this PR under your name, no attribution required. Just use it. |
Yes, it seems like the best approach |
|
So @mntns, this code should still work: // merge 2 objects. y has precedence over x.
fn merge_objects(x: &mut ValueMap, y: &ValueMap) -> Option<()> {
// we need to iterate over both keys, so we need a full deduplication
let keys: BTreeSet<String> = x.keys().chain(y.keys()).cloned().collect();
for key in keys {
let in_x = x.contains_key(&key);
let in_y = y.contains_key(&key);
if !in_x && in_y {
let yk = y[&key].clone();
x.insert(key, yk);
continue;
} else if !in_y {
continue;
}
let xk = x.get_mut(&key)?;
let yk = y.get(&key)?;
if xk.is_null() && !yk.is_null() {
*xk = yk.clone();
continue;
} else if yk.is_null() {
continue;
}
// now we've filtered out missing keys and optional values
// all key/value pairs should be same type.
if xk.is_object() {
merge_objects(xk.as_object_mut()?, yk.as_object()?)?;
} else {
*xk = yk.clone();
}
}
Some(())
}And now that arrays are optional, it will work correctly in all cases. Once this is implemented it lgtm and is ready for merging. |
|
Hi everyone, is there any update? The new version release seems to be stuck here? How long will it take? |
|
I didn't have time during the past few days, but integrated the new merging code by @Alexhuszagh now (thanks!) and improved the parse function signatures that @Emilgardis proposed. As far as I'm aware, the only thing that is missing is that |
|
I've rebased the pr here: https://github.com/Emilgardis/cross/tree/pr/mergetoml754 I can push that onto here if you want I have some small things to comment on, ill do that shortly I think we can implement the cargo metadata things in another pr :D |
|
im excited for this, thanks @mntns for doing this! |
|
Thanks for the review/rebase, @Emilgardis. Yes, please push it, then I'll make the requested changes. |
|
I accidentally pushed my main to your main I think 😅 |
|
github!!! @mntns I'm unable to reopen & can't push to your branch anymore. Here's how you can fix it. either add me with access to your fork and I'll fix it, or on your local repo: when you've done that you can reopen the pr |
|
@Emilgardis, just pulled your |
|
Reopen button is greyed out new pr: #842 |
|
you can't reöpen a PR if you have force pushed to it (i think just before closing it) |
Alexhuszagh
added
enhancement
A-config
This addresses #657 and enables that
crosscan be configured via package metadata in theCargo.toml.