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.

Sign up for GitHub

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

Jump to bottom

qt5.{qtwebengine, qtwebkit}: fix -Wno-class-memaccess warning for clang #70544

Merged
merged 1 commit into from
Oct 8, 2019
Merged

qt5.{qtwebengine, qtwebkit}: fix -Wno-class-memaccess warning for clang #70544

merged 1 commit into from
Oct 8, 2019
+4 −5

Conversation

d-goldin
Copy link
Contributor

@d-goldin d-goldin commented Oct 6, 2019 •
edited
Loading

Motivation for this change

With a previous fix for log size issues due to GCC 8
a gcc specific -W flag was added that clang does
not gracefully ignore.

See comment #70544 (comment)
This does not break the build, it just logs unneeded warnings and the breakage is somewhere else.

This was done in #68434 and #68869

We forgot to test this with clang and/or on darwin.

cc @worldofpeace

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

Sorry, something went wrong.

@d-goldin d-goldin requested a review from ttuegel as a code owner October 6, 2019 22:41
@ofborg ofborg bot requested review from matthewbauer, periklis and abbradar October 6, 2019 22:56
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 6, 2019
@d-goldin
Copy link
Contributor Author

d-goldin commented Oct 6, 2019 •
edited
Loading

Links to broken builds:

https://hydra.nixos.org/build/102530965
https://hydra.nixos.org/build/102399928

@veprbl veprbl added the 9.needs: port to stable A PR needs a backport to the stable release. label Oct 6, 2019
worldofpeace
worldofpeace reviewed Oct 6, 2019
View reviewed changes
pkgs/development/libraries/qt-5/modules/qtwebkit.nix
@@ -54,9 +54,9 @@ qtModule {
[
# with gcc7 this warning blows the log over Hydra's limit
"-Wno-expansion-to-defined"
Copy link
Contributor

Choose a reason for hiding this comment

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

But this one gets ignored?

Copy link
Contributor Author

@d-goldin d-goldin Oct 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

No, this one is actually supported in clang: https://clang.llvm.org/docs/DiagnosticsReference.html#wexpansion-to-defined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify my phrasing/use of "ignore" - with GCC it's the default to ignore non-existent warning options. With clang it's afaik not. There is -Wno-unknown-warning-option for that, which we don't use as far as I can tell.

@worldofpeace
Copy link
Contributor

@GrahamcOfBorg build qt5.qtwebengine

@worldofpeace
Copy link
Contributor

hmm, I guessing will probably timeout on darwin and ofborg in general.
So I couldn't test that.

A darwin tester is probably needed (or builder access).

@d-goldin
Copy link
Contributor Author

d-goldin commented Oct 7, 2019

@worldofpeace: I briefly tried it out running the beginning of the build on NixOS by overriding stdenv with pkgs.clangStdenv for both and it did not immediately die. But I'm currently traveling and don't have access to a machine sufficiently snappy to run those compiles fast enough.

@worldofpeace
Copy link
Contributor

Ahh @d-goldin, I guess it's sufficient to switch it out for clang. Will try that 👍

@worldofpeace
Copy link
Contributor

Hmm, I'm not sure how I can do that with these qt expressions...

@d-goldin
Copy link
Contributor Author

d-goldin commented Oct 7, 2019 •
edited
Loading

@worldofpeace: I tried it with nix-build -v -E 'with import <nixpkgs> {}; (lib.overrideDerivation pkgs.qt5.qtwebkit (oldAttrs: { stdenv = pkgs.clangStdenv; }))'

@d-goldin
Copy link
Contributor Author

d-goldin commented Oct 7, 2019 •
edited
Loading

Uhm, but you know what - I looked at the log again, and I got misled by the warning about the wrong flag. This does not actually seem to cause issues with the build itself (at least log length due to warning does not seem problematic either at this stage) so I guess it's a "nice to make it cleaner" but did not actually break the builds. Really sorry for the hassle, my bad :(

@d-goldin d-goldin changed the title qt5.{qtwebengine, qtwebkit}: fix build on clang qt5.{qtwebengine, qtwebkit}: fix -Wno-class-memaccess warning for clang Oct 7, 2019
@d-goldin
qt5.{qtwebengine, qtwebkit}: fix -Wno-class-memaccess warning for clang
3bfe087 
With a previous fix for log size issues due to GCC 8
a gcc specific `-W` flag was added that clang does
not know, so it spams the logs.
@d-goldin d-goldin force-pushed the fix_qtweb_warning_clang branch from fa3375a to 3bfe087 Compare October 7, 2019 02:13
veprbl
veprbl reviewed Oct 7, 2019
View reviewed changes
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Tested this on darwin. As expected, this doesn't fix the builds.

ttuegel
ttuegel approved these changes Oct 8, 2019
View reviewed changes
Copy link
Member

@ttuegel ttuegel 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 to me!

@worldofpeace worldofpeace merged commit 392eebd into NixOS:master Oct 8, 2019
@d-goldin d-goldin deleted the fix_qtweb_warning_clang branch October 8, 2019 19:24
@worldofpeace
Copy link
Contributor

backported in 9bbad4c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veprbl veprbl

@worldofpeace worldofpeace

@ttuegel ttuegel

@matthewbauer matthewbauer

@periklis periklis

@abbradar abbradar

Assignees
No one assigned
Labels
6.topic: qt/kde 9.needs: port to stable A PR needs a backport to the stable release. 10.rebuild-darwin: 11-100 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@d-goldin @worldofpeace @veprbl @ttuegel