-
-
Notifications
You must be signed in to change notification settings - Fork 312
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
Reenable CI fuzzing of pull requests #1596
Conversation
This reverts commit 3d90ab0.
As before GitoxideLabs#1536. I'm not sure CI fuzzing actually uses locked dependencies, though.
@Byron In EliahKagan#1 (comment) you had suggested that downgrading
|
Thanks for looking into this! Admittedly I completely forget about it already and CI-Fuzzing would be lost in time.
It's true, I also have the impression it uses the the latest possible versions of dependencies, so Cargo.lock shouldn't matter for it. So updating the image to use a more recent Rust version certainly would help. On top of that, I think the minimal |
Since around GitoxideLabs/gitoxide#1536, fuzzing is broken for `gitoxide` due to an error related to `serde`. As shown there and in GitoxideLabs/gitoxide#1596, the error is: error[E0658]: `#[diagnostic]` attribute name space is experimental --> /rust/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.210/src/de/mod.rs:536:5 | 536 | diagnostic::on_unimplemented( | ^^^^^^^^^^ | = note: see issue #111996 <rust-lang/rust#111996> for more information = help: add `#![feature(diagnostic_namespace)]` to the crate attributes to enable = note: this compiler was built on 2024-02-11; consider upgrading it if it is out of date Since rust-lang/rust#111996 is closed as completed, and similar errors appear to have been fixed in oss-fuzz for other projects by using the latest nightly toolchain, this makes the same change for `gitoxide` as was made in - google#12404 for `starlark-rust` - google#12409 for `rhai` See also google#12410.
I've opened google/oss-fuzz#12512 for this. I assume fuzzing is completely broken and not just when triggered by our CI, but I have only actually verified that it is broken on CI. I'm not sure how to check other runs, or even if I am able to do so, since I have no special access. (I have access to oss-fuzz reports for GitPython but not, as far as I know, for gitoxide.) If that change is accepted and if it fixes the problem, this PR could then be merged to reenable CI fuzzing of pull requests, but if that is to be done, then the second commit here should either be dropped via force push, or reverted. This is because downgrading To aid with that, I've gone ahead and reverted that commit. But we can still force push back to the first commit, of course.
Based on serde-rs/serde#2770, and specifically serde-rs/serde#2770 (comment), it looks like the issue is known and only occurs when using old nightly compilers, and that Eventually, I think the default version oss-fuzz uses of the nightly Rust compiler will be upgraded. See google/oss-fuzz#12410. But we shouldn't have to wait for that to happen to fix this. As noted there and in google/oss-fuzz#12512, the latest nightly is being used in a couple of projects to work around this (google/oss-fuzz#12404, google/oss-fuzz#12409). |
Since around GitoxideLabs/gitoxide#1536, fuzzing is broken for `gitoxide` due to an error related to `serde`. As shown there and in GitoxideLabs/gitoxide#1596, the error is: error[E0658]: `#[diagnostic]` attribute name space is experimental --> /rust/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.210/src/de/mod.rs:536:5 | 536 | diagnostic::on_unimplemented( | ^^^^^^^^^^ | = note: see issue #111996 <rust-lang/rust#111996> for more information = help: add `#![feature(diagnostic_namespace)]` to the crate attributes to enable = note: this compiler was built on 2024-02-11; consider upgrading it if it is out of date Since rust-lang/rust#111996 is closed as completed, and similar errors appear to have been fixed in oss-fuzz for other projects by using the latest nightly toolchain, this makes the same change for `gitoxide` as was made in: - google#12404 for `starlark-rust` - google#12409 for `rhai` See also : - google#12410 - serde-rs/serde#2770
…ml`" This reverts commit aafa8b1, since fuzzing doesn't use locked dependencies, so that's not a fix. See: - GitoxideLabs#1596 (comment) - google/oss-fuzz#12512
Thanks a lot for the summary, I am looking forward to upstream merge so CI will be back here. |
Since around GitoxideLabs/gitoxide#1536, fuzzing is broken for `gitoxide` due to an error related to `serde`. As shown there and in GitoxideLabs/gitoxide#1596, the error is: error[E0658]: `#[diagnostic]` attribute name space is experimental --> /rust/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.210/src/de/mod.rs:536:5 | 536 | diagnostic::on_unimplemented( | ^^^^^^^^^^ | = note: see issue #111996 <rust-lang/rust#111996> for more information = help: add `#![feature(diagnostic_namespace)]` to the crate attributes to enable = note: this compiler was built on 2024-02-11; consider upgrading it if it is out of date Since rust-lang/rust#111996 is closed as completed, and similar errors appear to have been fixed in oss-fuzz for other projects by using the latest nightly toolchain, this makes the same change for `gitoxide` as was made in: - #12404 for `starlark-rust` - #12409 for `rhai` See also: - #12410 - serde-rs/serde#2770 cc @Byron
…ml`" This reverts commit aafa8b1, since fuzzing doesn't use locked dependencies, so that's not a fix. See: - GitoxideLabs#1596 (comment) - google/oss-fuzz#12512
…ml`" This reverts commit aafa8b1, since fuzzing doesn't use locked dependencies, so that's not a fix. See: - GitoxideLabs#1596 (comment) - google/oss-fuzz#12512, which does fix it
This is working now, since google/oss-fuzz#12512. If this pull request is merged, then it might be a good idea, afterwards, to add the CIFuzz job as a required check for automatic merging. Considerations:
Breakages (if they do not reflect a possible bug due to changes a PR proposes) can reasonably be addressed by temporarily disabling CI fuzzing, as was done in #1536. (In unusual cases, they could possibly be addressed by manually merging pull requests for which fuzzing has failed but other checks have passed.) This pull request does not attempt to introduce changes to cause fuzzing to be effectively required, because I don't know of a way to do that by editing workflows that is clearly better than configuring it manually. #1536, which (among other changes) removed the CIFuzz job that this PR restores, was merged on August 23, which was before #1551 was opened and merged on August 25. Reviewing #1551 therefore did not involve any considerations of whether fuzzing failures should block PRs. Because the CIFuzz job is in a different GitHub Actions workflow file from the jobs affected by #1551, a cross-workflow dependency would be needed to include it in a similar way. Since this is not necessarily clearer or more elegant than configuring it as required, I have not made that change here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, that's great news!
Then it seems that after merging, this job will be made a required one to allow it to block merges, accepting the Risk that eventually, it will fail for other reasons.
Could you tune the fuzz-seconds
fields to bring the job down to the longest running non-fuzz job? The suggestion should be fine, but might need more tuning.
Per EliahKagan#1 (comment), it looks like I can't investigate this properly in a fork-internal PR. Ideally this will turn into a mergeable PR, but that is not the case yet, and it is not guaranteed.
The error is:
As noted in EliahKagan#1 (comment), this might be possible to fix by downgrading a dependency, though I would be reluctant to modify versions in
Cargo.lock
directly, which I expect could be undone automatically when changes are made to anyCargo.toml
file anywhere in the workspace.What I am not sure of is whether it should be done that way, or by using a newer toolchain by updating the Docker image. rust-lang/rust#111996 is closed as completed. Does that mean using a newer
rustc
would avoid the problem?