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

Incorrect Parsing of Arrays in Config Files #863

Closed
1 of 11 tasks
Alexhuszagh opened this issue Jun 26, 2022 · 6 comments · Fixed by #867
Closed
1 of 11 tasks

Incorrect Parsing of Arrays in Config Files #863

Alexhuszagh opened this issue Jun 26, 2022 · 6 comments · Fixed by #867
Assignees
Milestone

Comments

@Alexhuszagh
Copy link
Contributor

Checklist

Describe your issue

If providing arrays in [build.env], they don't seem to be used correctly. For example, the following config files:

Case 1

[build.env]
passthrough = ["CARGO_TERM_COLOR"]

Case 2

[build.env]
passthrough = ["CARGO_TERM_COLOR"]

[target.aarch64-unknown-linux-gnu.env]
passthrough = []

Case 3

[build.env]
passthrough = []

[target.aarch64-unknown-linux-gnu.env]
passthrough = ["CARGO_TERM_COLOR"]

When compiling with CARGO_TERM_COLOR=never cross build --target aarch64-unknown-linux-gnu, we should expect CARGO_TERM_COLOR to passthrough in all cases, but it only does in the last one. We'd expect in case 2 and 3 for the arrays to merge, and in 1 for it to only use the passthrough in [build.env]. It seems to ignore case 1 and 2 though.

What target(s) are you cross-compiling for?

No response

Which operating system is the host (e.g computer cross is on) running?

  • macOS
  • Windows
  • Linux / BSD
  • other OS (specify in description)

What architecture is the host?

  • x86_64 / AMD64
  • arm32
  • arm64 (including Mac M1)

What container engine is cross using?

  • docker
  • podman
  • other container engine (specify in description)

cross version

cross 0.2.2

Example

No response

Additional information / notes

No response

@Alexhuszagh
Copy link
Contributor Author

We also need to add unittests for this to avoid further regressions.

@Emilgardis
Copy link
Member

Emilgardis commented Jun 26, 2022

In case two, I don't think it should merge/work

@Alexhuszagh
Copy link
Contributor Author

In case two, I don't think it should merge/work

Hmmm, I think that would be a breaking change because we have the sum of the values, correct? Maybe something to change in 0.3.

cross/src/config.rs

Lines 303 to 329 in 48d2b7b

fn sum_of_env_toml_values<'a>(
&'a self,
env_values: Option<impl AsRef<[String]>>,
toml_getter: impl FnOnce(&'a CrossToml) -> (Option<&'a [String]>, Option<&'a [String]>),
) -> Result<Option<Vec<String>>> {
let mut defined = false;
let mut collect = vec![];
if let Some(vars) = env_values {
collect.extend(vars.as_ref().iter().cloned());
defined = true;
} else if let Some((build, target)) = self.toml.as_ref().map(toml_getter) {
if let Some(build) = build {
collect.extend(build.iter().cloned());
defined = true;
}
if let Some(target) = target {
collect.extend(target.iter().cloned());
defined = true;
}
}
if !defined {
Ok(None)
} else {
Ok(Some(collect))
}
}
}

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jun 26, 2022

It looks like env.passthrough is set to true too (but the rest are false):

cross/src/config.rs

Lines 250 to 257 in 48d2b7b

pub fn env_passthrough(&self, target: &Target) -> Result<Option<Vec<String>>> {
self.vec_from_config(
target,
Environment::passthrough,
CrossToml::env_passthrough,
true,
)
}

@Emilgardis
Copy link
Member

hmm, I guess we need a better way to flag that that nothing should be inherited for arrays, for now I think it's fine to keep the correct behaviour (i.e all cases should work as described)

the issue is that arrays can be null, empty or filled. Strings and booleans are null or filled.

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jun 26, 2022

I've fixed all of the above in #867, and added comprehensive unittests to ensure the regression does not reoccur. In v0.3.0, we can set it so they don't merge.

@Alexhuszagh Alexhuszagh added this to the v0.2.3 milestone Jun 26, 2022
@bors bors bot closed this as completed in #867 Jun 26, 2022
libcommon added a commit to libcommon/template-repo-rs that referenced this issue Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants