Don't leave root during bench-check.

[larsraph]

Error

During $ cargo run -p ci -- compile

$ cargo check --benches --target-dir ../target --manifest-path ./benches/Cargo.toml
error: failed to create directory `D:\bevy\..`

Caused by:
  Access is denied. (os error 5)

thread 'main' (16628) panicked at tools\ci\src\ci.rs:62:13:
One or more CI commands failed:
bench-check: Failed to check the benches.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Solution

• Instead of accessing ../target access ./target

Testing

• Now the command works on my device.

[SpecificProtagonist]

As far as I can tell, this bug originates from #12923, before which the cargo command was run from the benches/ directory.

The proper fix is to not specify the target directory at all. target is already the default, and specifying it as an argument here overwrites any custom target directory when run locally (e.g. I have my target directory on a different drive).

[larsraph]

The proper fix is to not specify the target directory at all. target is already the default

Thanks! Fixed.

I also fixed incorrect paths in CompileFail comments.

[larsraph]

So the CompileFail ci tests use a custom override (because they are independent crates) back to ./target. This should be completely fine 99% of the time and I don't think it's worth figuring out how to change but we could try to make sure any override applies to them as well as well.

[OniriqueCherry]

As far as I can tell, this bug originates from #12923, before which the cargo command was run from the benches/ directory.

The proper fix is to not specify the target directory at all. target is already the default, and specifying it as an argument here overwrites any custom target directory when run locally (e.g. I have my target directory on a different drive).

yeah #12923 lines up with what I dug up. fuller chain:

• [Merged by Bors] - CI tool usage #3876 (2022): bench-check ran used to run inside pushd("benches") with --target-dir ../target, so ../target resolved to the main workspace's ./target/. The goal was to to share artifacts with main so the bevy crate graph + their crates.io deps don't get rebuilt into a second target dir every CI run i think.
• tools: Refactor CI to use argh #12923: pushd got dropped, but it was still left with ../target. from repo root so it resolved outside the repo wich made cargo lost the manifest, and target-dir started writing one level above the repo.
• Fix CI bench compile check #14728: added --manifest-path ./benches/Cargo.toml to fix the manifest part. but it left ../target, which kinda made it was not even using the compiled cached artifacts since but it's fatal on windows-at-drive-root since no permission, which ig is larsraph's bug.

With this proposed fix the target will go to targets/benches, so it will fix the bug but it will compile for nothing again instead using the cached artifacts in ./target no ?

Maybe there is a better fix that allows us to not override custom targets but still keep the original intent of not compiling artifacts again.

[larsraph]

targets/benches

I was very confused for a minute but you meant benches/target right?

I just tried it and it didn't create a benches/target folder as well as getting compiled instantly. I would guess that specifying --manifect-path doesn't override --target-dir to match.

[OniriqueCherry]

targets/benches

I was very confused for a minute but you meant benches/target right?

I just tried it and it didn't create a benches/target folder as well as getting compiled instantly. I would guess that specifying --manifect-path doesn't override --target-dir to match.

really i thought workspace created their own sub folder in the target directory

[larsraph]

Yea but we're not using --package within a workspace we're literally overriding what project (workspace? is it a workspace with only one package?) we're using by changing the manifest path.

[larsraph]

I also changed the compile-fail tests to use the manifest-path as well.

The result should mean that any root level override to the target should apply to all CI tests as they all invoke cargo at the root and don't manually override the target-dir.

[OniriqueCherry]

Yea but we're not using --package within a workspace we're literally overriding what project (workspace? is it a workspace with only one package?) we're using by changing the manifest path.

Oh my bad i think it was historically a separate workspace, now it's a member, the proposed changes look good for me.

It fixes the bug, and allow custom override, the CLI should be faster as a consequence to.

[larsraph]

To clarify I don't think it matters if it's a member or if it's a separate workspace. I think cargo by default uses the target folder of the active directory (unless overridden). AFAIK the target folder is NOT part of a project/workspace. So if we run cargo from a certain directory and tell it to use the manifest of a completely different crate it should use the same target directory while using the manifest and src directories of the actual library.

[SpecificProtagonist]

I think cargo by default uses the target folder of the active directory (unless overridden).

Have you tried this? Cargo defaults to using a target dir relative to the manifest. That's why you can call cargo from subdirectories without problem. Though bench_check & compile_fail expect to be run from the root and panic.

It seems like unfortunately there isn't a (reasonable) way to find the target directory. So I think we need to go with your original fix of passing --target-dir target.

Could you also fix the bevy_macros → bevy_derive please?

[larsraph]

bevy_macros → bevy_derive

Good catch!

To be clear I did test it and it was working... buuut... My previous explanation for why it was working was dead wrong. I made assumptions based on the API and my testing. That's my bad. Thanks for pointing it out and leading me to actually look it up.

Turns out the entire things been overcomplicated. Any packages in a workspace automatically share build artefacts. The reason it was working was because cargo was smart enough to realize that. I've simplified the CI tests to just execute cargo with no --target-dir using the package name and confirmed that artefacts are being shared*.

*Except the integration tests which are explicitly excluded from the workspace and which never shared artefacts in the first place.

[larsraph]

Thanks for helping me out SpecificProtagonist and OniriqueCherry!