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

Conditionally compile gix-revision at_symbol fuzzed test #1434

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

EliahKagan
Copy link
Member

This allows compilation to succeed when building tests on 32-bit Windows systems, and probably other 32-bit systems/builds.

This test function had been set as ignored in 32-bit builds with the cfg_attr attribute. But it was still compiled, and the compilation failed on systems where the literal 9223372036854775808 is too big for usize.

This was due to the implicit #[deny(overflowing_literals)]. While that could be conditionally suppressed for 32-bit builds, the test would not really be meaningful. So this leaves that diagnostic in place, and turns the conditional ignore into conditional compilation.


Due to rust-lang/libz-sys#197, I ran the tests on the 32-but Windows 10 test system with:

cargo nextest run --no-default-features --features 'max-control,gix-features/zlib-stock,gitoxide-core-blocking-client,http-client-curl' --all --no-fail-fast

At the current tip of main (1e79c5c), no test cases actually ran, due to this compile error:

error: literal out of range for `usize`
  --> gix-revision\tests\spec\parse\anchor\at_symbol.rs:17:50
   |
17 |     assert_eq!(rec.nth_checked_out_branch, [Some(9223372036854775808), None]);
   |                                                  ^^^^^^^^^^^^^^^^^^^
   |
   = note: the literal `9223372036854775808` does not fit into the type `usize` whose range is `0..=4294967295`
   = note: `#[deny(overflowing_literals)]` on by default

error: could not compile `gix-revision` (test "revision") due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: command `'\\?\C:\Users\ek\.rustup\toolchains\stable-i686-pc-windows-msvc\bin\cargo.exe' test --no-run --message-format json-render-diagnostics --all --features max-control,gix-features/zlib-stock,gitoxide-core-blocking-client,http-client-curl --no-default-features` exited with code 101

See this file in this gist for full output. With the change made here, the build succeeds; although some tests fail, all of them are able to build (and most pass). It seems to me that the failures, which this change does not cause, should be considered outside the scope of this PR. The output on the 32-bit test system after the change here can be seen in this other file in the gist.

It seems to me that the value of compiling this test function on 32-bit systems where it is ignored is fairly small, such that it would not justify weakening the compiler's diagnostics, adding an explicit conversion, or significant changes to the test code. Furthermore, I don't think this can be fixed by adding a suffix to change the type of the literal. The literal will be accepted if suffixed with u64, but then compilation still fails because the comparison cannot be performed.

To decide whether the #[test] or #[cfg(...)] attribute should appear first, I examined which style was more common in the codebase by counting matches of the regular expressions #\[test.+\n#\[cfg\( and #\[cfg\(.+\n#\[test. I did not include any style cleanup here, and I am not convinced the small number of cases where #[cfg(...)] comes first are unintentional, but I went with the prevailing style.

I don't know of a way to include the explanation as metadata when using #[cfg(...)] so I just made it a comment.

This allows compilation to succeed when building tests on 32-bit
Windows systems, and probably other 32-bit systems/builds.

This test function had been set as ignored in 32-bit builds with
the `cfg_attr` attribute. But it was still compiled, and the
compilation failed on systems where the literal 9223372036854775808
is too big for `usize`.

This was due to the implicit `#[deny(overflowing_literals)]`. While
that could be conditionally suppressed for 32-bit builds, the test
would not really be meaningful. So this leaves that diagnostic in
place, and turns the conditional ignore into conditional
compilation.
@Byron
Copy link
Member

Byron commented Jul 1, 2024

Thanks a lot, appreciated!

I don't know of a way to include the explanation as metadata when using #[cfg(...)] so I just made it a comment.

Unfortunately there is no way I know of to produce a compiler warning or message. There is only a way to produce a compile error.

To decide whether the #[test] or #[cfg(...)] attribute should appear first, I examined which style was more common in the codebase by counting matches of the regular expressions #\[test.+\n#\[cfg\( and #\[cfg\(.+\n#\[test. I did not include any style cleanup here, and I am not convinced the small number of cases where #[cfg(...)] comes first are unintentional, but I went with the prevailing style.

For this I believe there is no intentional order. There only ordering in that regard that I know of is that that doc-comments always come first, followed by attributes. So it's…

/// Docs
#[cfg(foo]
#[derive(Debug)]
struct Foo;

…rather than…

#[cfg(foo]
#[derive(Debug)]
/// Docs
struct Foo;

@Byron Byron merged commit 85019d0 into GitoxideLabs:main Jul 1, 2024
19 checks passed
@EliahKagan EliahKagan deleted the fix-32bit-test-build branch July 1, 2024 07:25
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