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

glog: disable broken test under clang #248537

Merged
merged 1 commit into from
Aug 13, 2023

Conversation

tobim
Copy link
Contributor

@tobim tobim commented Aug 11, 2023

Description of changes

google/glog#937

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@r-burns r-burns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I think even more failing tests have come up since I last looked at this. With doCheck = true on x86_64-darwin, I see failures in logging_custom_prefix, logging, and mock-log.

pkgs/development/libraries/glog/default.nix Show resolved Hide resolved
@tobim
Copy link
Contributor Author

tobim commented Aug 12, 2023

The latest version of the patch disables the ones you mentioned explicitly now, can you check again?

Comment on lines 42 to 44
"logging_custom_prefix"
"logging"
"mock-log"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike ctest -E, looks like the GTEST_FILTER needs the specific test case name:

Suggested change
"logging_custom_prefix"
"logging"
"mock-log"
"LogBacktraceAt.DoesBacktraceAtRightLineWhenEnabled"

And mock-log seems to be segfaulting at init so unfortunately I think we would need a separate mechanism to disable that test file entirely, such as ctest -E as is done here

With LogBacktraceAt.DoesBacktraceAtRightLineWhenEnabled and mock-log disabled, all the remaining tests pass for me on x86_64-darwin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I implemented the exclusion according to your recommendation.

@tobim
Copy link
Contributor Author

tobim commented Aug 13, 2023

I added a workaround to a broken glogConfig.cmake installation with the latest commit.

Copy link
Contributor

@r-burns r-burns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Tested and working on x86_64-darwin. Sorry about all that ctest nonsense.

@r-burns r-burns merged commit d232c2a into NixOS:staging Aug 13, 2023
8 of 10 checks passed
@tobim tobim deleted the pkgs/glog-fix-clang16 branch August 14, 2023 09:49
@vcunat
Copy link
Member

vcunat commented Aug 26, 2023

Some of the tests crash on *-darwin now:
https://hydra.nixos.org/build/232481702/nixlog/2/tail

@tobim
Copy link
Contributor Author

tobim commented Aug 26, 2023

Too bad. I guess we'll have to disable again.

@tjni
Copy link
Contributor

tjni commented Aug 27, 2023

I don't know if this is the best way to disable the tests, but it suffices to skip them locally on my aarch64-darwin machine: #251729. I'm happy to iterate on this to make this better moving forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants