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

Jump to bottom

glib,gtk: correct CLAGS for plain buildtype #65850

Merged
merged 5 commits into from Aug 5, 2019
Merged

glib,gtk: correct CLAGS for plain buildtype #65850

merged 5 commits into from Aug 5, 2019

Conversation

worldofpeace
Copy link
Contributor

Motivation for this change

A little fallout from #58310 but this should correct things to what
I believe they were or should have been.

Noticed this fairly quickly because of #65843.

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.

@ofborg ofborg bot requested review from 7c6f434c, lovek323 and vcunat August 3, 2019 15:37
@worldofpeace worldofpeace mentioned this pull request Aug 4, 2019
Merged
@worldofpeace
Copy link
Contributor Author

Guess some discussion might be happening upstream

@worldofpeace
Copy link
Contributor Author

worldofpeace commented Aug 4, 2019 •
edited

Looks like we should do G_DISABLE_CAST_CHECKS in gnome-settings-daemon too

Edit: GNOME 3.34 we might want to set this

@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: pantheon The Pantheon desktop environment labels Aug 4, 2019
jtojnar
jtojnar reviewed Aug 5, 2019
View reviewed changes
pkgs/development/libraries/glib/default.nix Outdated
# Default for release buildtype but passed manually because
# we're using plain
"-DG_DISABLE_CAST_CHECKS"
] ++ optional stdenv.isSunOS "-DBSD_COMP";
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea what is this for? It originated in 8ae2a89 but I do not see any mentions in git log -p -SBSD_COMP in Glib repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I have no idea either, since we've diverged so much from then I think we can just drop it.

@jtojnar
Copy link
Contributor

jtojnar commented Aug 5, 2019

Could you add links to relevant sections of meson.build to the other commit messages as well?

Also please describe in the gtk3 commit message why we need G_ENABLE_DEBUG. It will be very helpful in the future, when I will be trying to figure out which flags to prune using git blame.

Otherwise, it looks fine.

@worldofpeace
Copy link
Contributor Author

Could you add links to relevant sections of meson.build to the other commit messages as well?

Also please describe in the gtk3 commit message why we need G_ENABLE_DEBUG. It will be very helpful in the future, when I will be trying to figure out which flags to prune using git blame.

Otherwise, it looks fine.

Yep, will do all of that.

@worldofpeace
gtk3: defines for debugging, disable cast checks
27a71a1 
Because we're using plain buildtype these have to be
passed manually.

See: https://gitlab.gnome.org/GNOME/gtk/blob/3.24.10/meson.build#L59

With autotools the mapping for the the options to the defines was:

yes:     G_ENABLE_DEBUG G_ENABLE_CONSISTENCY_CHECKS
minimum: G_ENABLE_DEBUG G_DISABLE_CAST_CHECKS
no:      G_DISABLE_CAST_CHECKS G_DISABLE_ASSERT G_DISABLE_CHECKS

So we're passing the exact ones that would've been used for minimum.

Additionally it isn't a good idea to pass the equivalents used for "no" 
as it eliminates G_ENABLE_DEBUG which disables pre-condition checks and 
assertions. The actual option only existed to serve people who needed a 
specific build of GTK for very specific environments. And now they are
much better served with meson's plain buildtype and figuring out what to
pass themselves.
@worldofpeace
glib: add cflag G_DISABLE_CAST_CHECKS
60266eb 
This is what would have been passed before with the release
buildtype.

See: https://gitlab.gnome.org/GNOME/glib/blob/2.60.4/meson.build#L208
@worldofpeace
gnome3.gnome-settings-daemon: add cflag G_DISABLE_CAST_CHECKS
c7c727f 
This is what would have been passed before with the release
buildtype.

See: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/blob/GNOME_SETTINGS_DAEMON_3_32_1/meson.build#L73
@worldofpeace
pantheon.elementary-settings-daemon: add cflag G_DISABLE_CAST_CHECKS
3b085b4 
Mirror c7c727f.
@worldofpeace
Copy link
Contributor Author

@jtojnar rewritten/revised

@worldofpeace
glib: drop define BSD_COMP
f2eddec 
I fail to see where or for what it is useful for.
@ofborg ofborg bot requested review from jtojnar and 7c6f434c August 5, 2019 10:46
@worldofpeace worldofpeace merged commit a2253f4 into NixOS:staging Aug 5, 2019
@worldofpeace worldofpeace deleted the bye-g-disable-checks branch August 5, 2019 15:33
@worldofpeace worldofpeace mentioned this pull request Aug 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar approved these changes

@hedning hedning Awaiting requested review from hedning

@lovek323 lovek323 Awaiting requested review from lovek323

@vcunat vcunat Awaiting requested review from vcunat

@7c6f434c 7c6f434c Awaiting requested review from 7c6f434c

Assignees
No one assigned
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: pantheon The Pantheon desktop environment 10.rebuild-darwin: 501+ 10.rebuild-darwin: 1001-2500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@worldofpeace @jtojnar @7c6f434c