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

Implement Cross.toml deserialization using serde #670

Merged
merged 11 commits into from
Mar 22, 2022
Merged

Conversation

mntns
Copy link
Contributor

@mntns mntns commented Mar 22, 2022

This PR implements the deserialization of the Cross.toml config using serde. It can therefore be seen as some preliminary work on the issues #657, #664, and is related to #532 on #624.

I added some basic documentation as module-level documentation, although I'm not sure whether this is the best place for that. Ideally it should be documentated in the README.md at some point. Furthermore, CrossToml lives in its own module, but a more suitable place might be the config module.

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.

looks great!

src/cross_toml.rs Outdated Show resolved Hide resolved
src/cross_toml.rs Outdated Show resolved Hide resolved
src/cross_toml.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@Emilgardis
Copy link
Member

I added some basic documentation as module-level documentation, although I'm not sure whether this is the best place for that. Ideally it should be documentated in the README.md at some point.

We can split the documentation out into it's own .md and then do

#![doc = include_str!("file.md")]

@mntns mntns requested a review from a team as a code owner March 22, 2022 19:08
@mntns
Copy link
Contributor Author

mntns commented Mar 22, 2022

Thanks for the prompt review, @Emilgardis! I made the suggested changes and split out the documentation in a separate .md file.

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.

some minor nits, not really too major

src/cross_toml.rs Show resolved Hide resolved
src/cross_toml.rs Outdated Show resolved Hide resolved
src/cross_toml.rs Outdated Show resolved Hide resolved
src/cross_toml.rs Show resolved Hide resolved
src/cross_toml.rs Outdated Show resolved Hide resolved
src/cross_toml.rs Outdated Show resolved Hide resolved
Emilgardis
Emilgardis previously approved these changes Mar 22, 2022
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.

this is great 😄 thank you!

I want to merge #624 as well, but I think it makes sense to merge this before

please address the remaining clippy issues and add a simple changelog entry and I'll r+ this!

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 Mar 22, 2022

Build succeeded:

@bors bors bot merged commit b40f3fc into cross-rs:main Mar 22, 2022
@Emilgardis Emilgardis mentioned this pull request Apr 6, 2022
bors bot added a commit that referenced this pull request Apr 7, 2022
683: fix env r=reitermarkus a=Emilgardis

Fixes a wrong assumption done in #670 where we went from

```toml
[target.aarch64-unknown-linux-gnu.env]
volumes = [
    "BUILD_DIR",
]
```

to

```toml
[target.aarch64-unknown-linux-gnu]
volumes = [
    "BUILD_DIR",
]
```

this fixes that, and makes sure that all markdown toml fences are checked in CI for correctness.

This pr also "un-opts" what was previously `CrossBuildConfig` to simplify a bit, see #670 (comment)

Co-authored-by: Emil Gardström <emil.gardstrom@gmail.com>
@Emilgardis Emilgardis added this to the v0.2.2 milestone Jun 15, 2022
@Alexhuszagh Alexhuszagh added the no-ci-targets PRs that do not affect any cross-compilation targets. label Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-ci-targets PRs that do not affect any cross-compilation targets.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants