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

Deny warnings on CI #1127

Closed
wants to merge 1 commit into from
Closed

Conversation

ouuan
Copy link
Contributor

@ouuan ouuan commented May 18, 2024

This mainly captures unused variables/imports related to ghost codes / no_std. Examples are #1121 and #1126.

Now the CI will fail, waiting for those two PRs.

@ouuan
Copy link
Contributor Author

ouuan commented May 19, 2024

There are some concerns at rust-lang/cargo#8424 regarding setting RUSTFLAGS. The main concern is that it may override rustflags in .cargo/config.toml. Now only the line_count tool uses a local .cargo/config.toml, and vargo is overriding the RUSTFLAGS env.

I'm not sure what's the best way to deny warnings on CI for Verus:

  • Setting RUSTFLAGS may break if we add a .cargo/config.toml in the future, and we might waste time finding out why it breaks.
  • Should we add -Dwarnings in vargo on CI?
  • Can we apply cargo clippy? It has a -D warnings command-line option.

@utaal
Copy link
Collaborator

utaal commented May 22, 2024

Hi @ouuan, thanks for the PR, but I don't have the bandwidth at the moment to look into this. One of the other maintainers may be able to have a look later.

At the current stage of development, I think warnings in CI are acceptable.

@ouuan
Copy link
Contributor Author

ouuan commented May 22, 2024

At the current stage of development, I think warnings in CI are acceptable.

Now we only have warnings with special compilation flags. It might be too strict if clippy is used, but the default warnings are easy to avoid.

This mainly captures unused variables/imports related to ghost codes.
Examples are verus-lang#1121 and verus-lang#1126.
@ouuan ouuan force-pushed the deny-warnings-on-ci branch from 43707ff to 5318677 Compare May 22, 2024 16:36
@ouuan ouuan marked this pull request as ready for review May 22, 2024 16:53
@utaal
Copy link
Collaborator

utaal commented May 28, 2024

We may be making some significant changes to the CI process soon, so I don't think this is the time to make this change (and I don't have time to look into the RUSTFLAGS issue) so I'll close this for now. If one of the other contributors @verus-lang/verus-devs wants to pick this up, please do.

@utaal utaal closed this May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants