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.

Sign up for GitHub

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

Jump to bottom

fix env #683

Merged
merged 4 commits into from
Apr 7, 2022
Merged

fix env #683

merged 4 commits into from
Apr 7, 2022

Conversation

Emilgardis
Copy link
Member

@Emilgardis Emilgardis commented Apr 6, 2022 •
edited
Loading

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

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

to

[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)

@Emilgardis Emilgardis requested a review from a team as a code owner April 6, 2022 08:32
@Emilgardis Emilgardis mentioned this pull request Apr 6, 2022
Closed
@Emilgardis Emilgardis added the no changelog A valid PR without changelog (no-changelog) label Apr 6, 2022
@Emilgardis Emilgardis force-pushed the fix-env branch 2 times, most recently from f70fa5b to ad583fb Compare April 6, 2022 08:46
@Emilgardis
Copy link
Member Author

Emilgardis commented Apr 6, 2022 •
edited
Loading

we don't have any tests for these, we should

@Emilgardis Emilgardis force-pushed the fix-env branch 2 times, most recently from 9e4cf23 to 2e55b1c Compare April 6, 2022 16:10
@Emilgardis
Copy link
Member Author

Emilgardis commented Apr 6, 2022 •
edited
Loading

https://github.com/cross-rs/cross/runs/5854579013?check_suite_focus=true#logs
image

Logs

/home/runner/.cargo/bin/cargo test --locked --all-targets --workspace
Compiling cross v0.2.1 (/home/runner/work/cross/cross)
Compiling ci v0.0.0 (/home/runner/work/cross/cross/ci)
Finished test [unoptimized + debuginfo] target(s) in 7.29s
Running unittests (target/debug/deps/ci-ac5447179e760b99)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

 Running tests/toml.rs (target/debug/deps/toml-ed5b2c07d81dec5b)

running 1 test
test toml_check ... FAILED

failures:

---- toml_check stdout ----
File: "/home/runner/work/cross/cross/docs/cross_toml.md"
testing snippet at: /home/runner/work/cross/cross/docs/cross_toml.md:6
testing snippet at: /home/runner/work/cross/cross/docs/cross_toml.md:17
testing snippet at: /home/runner/work/cross/cross/docs/cross_toml.md:26
testing snippet at: /home/runner/work/cross/cross/docs/cross_toml.md:37
File: "/home/runner/work/cross/cross/README.md"
testing snippet at: /home/runner/work/cross/cross/README.md:191
testing snippet at: /home/runner/work/cross/cross/README.md:203
testing snippet at: /home/runner/work/cross/cross/README.md:215
Warning: found unused key(s) in Cross configuration:

target.aarch64-unknown-linux-gnu.env.volume
thread 'toml_check' panicked at 'assertion failed: cross::cross_toml::CrossToml::parse(fence.as_str())?.1.is_empty()', ci/tests/toml.rs:44:13
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

failures:
toml_check

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s

error: test failed, to rerun pass '-p ci --test toml'
Error: The process '/home/runner/.cargo/bin/cargo' failed with exit code 101

🎉

@Emilgardis
Copy link
Member Author

Emilgardis commented Apr 6, 2022 •
edited
Loading

I've left a question over on the rust discord, about making it so that docs.rs shows
image

Seems it's not possible.

The reason I want this is for making it so that the additional dependencies do not follow, as dev-dependencies are not possible to feature gate. But maybe this is not as big of a concern as I'm thinking it is.

@Emilgardis
Copy link
Member Author

This is ready now, I opted out of trying to be smart with using another workspace member for this.

Instead just use normal integration test

Emilgardis
Emilgardis commented Apr 6, 2022
View reviewed changes
src/tests.rs
Comment on lines +21 to +31
let output = std::process::Command::new(
std::env::var("CARGO")
.ok()
.unwrap_or_else(|| "cargo".to_string()),
)
.arg("metadata")
.arg("--format-version=1")
.arg("--no-deps")
.current_dir(manifest_dir)
.output()
.unwrap();
Copy link
Member Author

@Emilgardis Emilgardis Apr 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Something like this is what we should probably actually use on/with #494

This was referenced Apr 6, 2022
Merged
Merged
Closed
reitermarkus
reitermarkus reviewed Apr 7, 2022
View reviewed changes
src/cross_toml.rs Outdated Show resolved Hide resolved
@Emilgardis
Copy link
Member Author

bors r=reitermarkus

@bors
Copy link
Contributor

bors bot commented Apr 7, 2022

Build succeeded:

@bors bors bot merged commit 416548a into cross-rs:main Apr 7, 2022
@Emilgardis Emilgardis deleted the fix-env branch April 7, 2022 18:29
@Emilgardis Emilgardis added this to the v0.2.2 milestone Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reitermarkus reitermarkus reitermarkus approved these changes

Assignees
No one assigned
Labels
no changelog A valid PR without changelog (no-changelog)
Projects
None yet
Milestone
v0.2.2
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Emilgardis @reitermarkus