Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Test build warnings #14

Closed
wants to merge 2 commits into from
Closed

Test build warnings #14

wants to merge 2 commits into from

Conversation

kennykerr
Copy link
Collaborator

Test fix for #13

@kennykerr kennykerr closed this Jul 13, 2021
@wravery
Copy link
Contributor

wravery commented Jul 13, 2021

@kennykerr I think it might have passed the checks because the run step is a separate invocation. If you put that together with the cargo test command in a multi-line script step it'll emit a batch script where the environment variable is set for that process.

@wravery
Copy link
Contributor

wravery commented Jul 13, 2021

...although I'm not sure how portable that is, pwsh or bash might be better. Another way is apparently to add it to .cargo/config: https://stackoverflow.com/questions/56870422/how-can-i-cause-compilation-to-fail-on-warnings-on-ci-and-have-extra-rustflags-s.

@riverar
Copy link
Collaborator

riverar commented Jul 13, 2021

@wravery Yup, working on it! We just discovered RUSTFLAGS are completely ignored when cross compiling which adds another dimension to the problem. 😂

@wravery
Copy link
Contributor

wravery commented Jul 13, 2021

Looks like a pretty old issue: rust-lang/cargo#4423, most of the movement on that seems to be around making sure that compiling with --target $host works the same as the default by special casing that triplet.

Would it be sufficient to ignore that difference and only break the build for the host triplet? It looks like the rust-lang/cargo#9322 PR was merged June 1st, so I would guess it's in the nightly branch at least. So if you set RUSTFLAGS consistently and it's ignored everywhere except when --target matches the host (and maybe only nightly for now), it should still fail at least one check.

@wravery
Copy link
Contributor

wravery commented Jul 13, 2021

Would it be sufficient to ignore that difference and only break the build for the host triplet? It looks like the rust-lang/cargo#9322 PR was merged June 1st, so I would guess it's in the nightly branch at least. So if you set RUSTFLAGS consistently and it's ignored everywhere except when --target matches the host (and maybe only nightly for now), it should still fail at least one check.

Ah, never mind. I just tried that against a local nightly toolchain and it doesn't work. Maybe it's not in nightly yet, or maybe I'm misunderstanding the fix in rust-lang/cargo#9322.

@riverar
Copy link
Collaborator

riverar commented Jul 13, 2021

@wravery In this case, we were working a bit too hard. We configured the default toolchain (i.e. x86 / x64) during CI but then also did a bit of extra work for cross-compilation (e.g. added targets via rustup, specified --targets during build) that wasn't needed and wasn't a noop as expected. (RUSTFLAGS is ignored!)

New PR #15 is failing expectedly now.

@kennykerr kennykerr deleted the warnings branch July 14, 2021 04:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants