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

libbacktrace: Add support for NIX_DEBUG_INFO_DIRS #207611

Merged
merged 4 commits into from
Mar 23, 2023

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented Dec 24, 2022

Description of changes

Clean up the expression, update and apply some patches to fix tests and make it work better on NixOS.

ianlancetaylor/libbacktrace@9b7f216...da7eff2

Includes two unmerged upstream patchsets:

cc @Twey

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.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@jtojnar
Copy link
Member Author

jtojnar commented Dec 24, 2022

Verified that this works well in conclusion with #200971.

By applying

preCheck = ''
  export NIX_DEBUG_INFO_DIRS=${glib.debug}/lib/debug:${gtk4.debug}/lib/debug
  export LD_PRELOAD=${libsegfault}/lib/libsegfault.so
  export SEGFAULT_SIGNALS=all
'';

I got

 0# (anonymous namespace)::segabort_handler(int) in /nix/store/qj2d2j1sl9qgfdj0yxdvwlxcq7l2sa2n-libsegfault-unstable-2022-11-13/lib/libsegfault.so
 1# 0x00007FFFF6E3DBF0 in /nix/store/ayfr5l52xkqqjn3n4h9jfacgnchz1z7s-glibc-2.35-224/lib/libc.so.6
 2# g_log_writer_default at ../glib/gmessages.c:2844
 3# g_log_structured_array at ../glib/gmessages.c:1967
 4# g_log_structured_standard at ../glib/gmessages.c:2053
 5# get_bus_address_dbus.constprop.0 at ../gtk/a11y/gtkatspicontext.c:1635
 6# get_bus_address.part.0 at ../gtk/a11y/gtkatspicontext.c:1715
 7# gtk_at_spi_create_context at ../gtk/a11y/gtkatspicontext.c:1760
 8# gtk_at_context_create at ../gtk/gtkatcontext.c:569
 9# gtk_widget_accessible_get_at_context at ../gtk/gtkwidget.c:8437
10# gtk_widget_init at ../gtk/gtkwidget.c:2368
11# g_type_create_instance at ../gobject/gtype.c:1917
12# g_object_new_internal at ../gobject/gobject.c:2228
13# g_object_new_valist at ../gobject/gobject.c:2567
14# g_object_new at ../gobject/gobject.c:2044
15# test_marker_layer_new in /build/source/build/tests/marker-layer
16# g_test_run_suite_internal at ../glib/gtestutils.c:3021
17# g_test_run_suite_internal at ../glib/gtestutils.c:3041
18# g_test_run_suite at ../glib/gtestutils.c:3115
19# g_test_run at ../glib/gtestutils.c:2234
20# main in /build/source/build/tests/marker-layer
21# __libc_start_call_main in /nix/store/ayfr5l52xkqqjn3n4h9jfacgnchz1z7s-glibc-2.35-224/lib/libc.so.6
22# __libc_start_main_impl in /nix/store/ayfr5l52xkqqjn3n4h9jfacgnchz1z7s-glibc-2.35-224/lib/libc.so.6
23# _start in /build/source/build/tests/marker-layer

compared to the following (without this patch set):

 0# (anonymous namespace)::segabort_handler(int) in /nix/store/qj2d2j1sl9qgfdj0yxdvwlxcq7l2sa2n-libsegfault-unstable-2022-11-13/lib/libsegfault.so
 1# 0x00007FFFF6E3DBF0 in /nix/store/ayfr5l52xkqqjn3n4h9jfacgnchz1z7s-glibc-2.35-224/lib/libc.so.6
 2# g_log_writer_default in /nix/store/61qy7d70070djqcg135l22v18a5py758-glib-2.74.3/lib/libglib-2.0.so.0
 3# g_log_structured_array in /nix/store/61qy7d70070djqcg135l22v18a5py758-glib-2.74.3/lib/libglib-2.0.so.0
 4# g_log_structured_standard in /nix/store/61qy7d70070djqcg135l22v18a5py758-glib-2.74.3/lib/libglib-2.0.so.0
 5# get_bus_address_dbus.constprop.0 in /nix/store/nxb37gpb5rikxrppjd72rc1d3x91w13l-gtk4-4.8.3/lib/libgtk-4.so.1
 6# get_bus_address.part.0 in /nix/store/nxb37gpb5rikxrppjd72rc1d3x91w13l-gtk4-4.8.3/lib/libgtk-4.so.1
 7# gtk_at_spi_create_context in /nix/store/nxb37gpb5rikxrppjd72rc1d3x91w13l-gtk4-4.8.3/lib/libgtk-4.so.1
 8# gtk_at_context_create in /nix/store/nxb37gpb5rikxrppjd72rc1d3x91w13l-gtk4-4.8.3/lib/libgtk-4.so.1
 9# gtk_widget_accessible_get_at_context in /nix/store/nxb37gpb5rikxrppjd72rc1d3x91w13l-gtk4-4.8.3/lib/libgtk-4.so.1
10# gtk_widget_init in /nix/store/nxb37gpb5rikxrppjd72rc1d3x91w13l-gtk4-4.8.3/lib/libgtk-4.so.1
11# g_type_create_instance in /nix/store/61qy7d70070djqcg135l22v18a5py758-glib-2.74.3/lib/libgobject-2.0.so.0
12# g_object_new_internal in /nix/store/61qy7d70070djqcg135l22v18a5py758-glib-2.74.3/lib/libgobject-2.0.so.0
13# g_object_new_with_properties in /nix/store/61qy7d70070djqcg135l22v18a5py758-glib-2.74.3/lib/libgobject-2.0.so.0
14# g_object_new in /nix/store/61qy7d70070djqcg135l22v18a5py758-glib-2.74.3/lib/libgobject-2.0.so.0
15# test_map_add_layers in /build/source/build/tests/map
16# g_test_run_suite_internal in /nix/store/61qy7d70070djqcg135l22v18a5py758-glib-2.74.3/lib/libglib-2.0.so.0
17# g_test_run_suite_internal in /nix/store/61qy7d70070djqcg135l22v18a5py758-glib-2.74.3/lib/libglib-2.0.so.0
18# g_test_run_suite in /nix/store/61qy7d70070djqcg135l22v18a5py758-glib-2.74.3/lib/libglib-2.0.so.0
19# g_test_run in /nix/store/61qy7d70070djqcg135l22v18a5py758-glib-2.74.3/lib/libglib-2.0.so.0
20# main in /build/source/build/tests/map
21# __libc_start_call_main in /nix/store/ayfr5l52xkqqjn3n4h9jfacgnchz1z7s-glibc-2.35-224/lib/libc.so.6
22# __libc_start_main_impl in /nix/store/ayfr5l52xkqqjn3n4h9jfacgnchz1z7s-glibc-2.35-224/lib/libc.so.6
23# _start in /build/source/build/tests/map

@RaitoBezarius
Copy link
Member

This should target staging IMHO. Putting in draft to avoid mass pings.

@RaitoBezarius RaitoBezarius marked this pull request as draft December 24, 2022 20:36
@jtojnar
Copy link
Member Author

jtojnar commented Dec 24, 2022

Why do you want to target staging? It is not very many rebuilds.

@RaitoBezarius
Copy link
Member

Why do you want to target staging? It is not very many rebuilds.

Sorry, confused 500+ and 101-500. Nevermind my comment.

@RaitoBezarius RaitoBezarius marked this pull request as ready for review December 24, 2022 22:29
jtojnar added 2 commits March 5, 2023 01:42
- Prefix version with “unstable-” as per contributing guide
- Do not use rec pointlessly
- Use `lib.enableFeature`
- Add update script
- Format the expression
- Remove unused callPackage input
jtojnar added 2 commits March 5, 2023 01:46
Only on Linux since they will fail on Darwin:

    dsymutil btest
    error: cannot parse the debug map for 'btest': The file was not recognized as a valid object file
    make[1]: *** [Makefile:2584: btest.dSYM] Error 1
pname = "libbacktrace";
version = "2020-05-13";
version = "unstable-2022-12-16";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this version is intended to be stable. I'm not sure exactly what to do about this as libbacktrace has weird version semantics: as the author said,

This is version 1.0. It is likely that this will always be version 1.0.

I'm not entirely sure how to fit this into the nixpkgs versioning scheme, but unstable doesn't seem to be it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@Twey Twey Mar 9, 2023

Choose a reason for hiding this comment

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

I think what I'm saying is that this is (notionally) a release. The initial release in that repository is release 1.0, and all commits on top of it are (unhelpfully named) patch versions of that release.

@@ -0,0 +1,42 @@
From a3b7510e4c9e7201a4301f2a45d8569b06354607 Mon Sep 17 00:00:00 2001
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen many patches in nixpkgs that include email metadata, but maybe someone else would like to weigh in on whether this is okay style or not. I don't really see the harm in it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s just what Git spits out.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there are a fair few patches in nixpkgs with author metadata (about 15%), so I guess it's fine.


# Support NIX_DEBUG_INFO_DIRS environment variable.
./0004-libbacktrace-Support-NIX_DEBUG_INFO_DIRS-environment.patch
];
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're adding patches, could we include ianlancetaylor/libbacktrace#92? This would be helpful for other nixpkgs packages to depend on libbacktrace without relying on the cc-wrapper include path generation.

Copy link
Member Author

@jtojnar jtojnar Mar 8, 2023

Choose a reason for hiding this comment

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

Would not other software need to rely on this for it to be useful? I would not expect other projects to rely on it until it is supported upstream (unless the developer does not realize they are relying on their distro’s patches).

Copy link
Contributor

Choose a reason for hiding this comment

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

In a non-Nix context pkgconfig is often not required because the libraries are in global directories, but within Nix I find that pkgconfig is very helpful to decouple packages' output structures.
I don't expect non-nixpkgs packages to expect pkgconfig for this, but rather it might be helpful for other nixpkgs packages that want to depend on libbacktrace. I've previously had to generate a pkgconfig manually for it and overrideAttrs.

Copy link
Member Author

Choose a reason for hiding this comment

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

In a non-Nix context pkgconfig is often not required because the libraries are in global directories

Right, and in Nixpkgs, the cc-wrapper plays a similar role.

I don't expect non-nixpkgs packages to expect pkgconfig for this, but rather it might be helpful for other nixpkgs packages that want to depend on libbacktrace.

We are usually trying to keep packages in Nixpkgs as close to upstream as possible. This typically means limiting patches to build or security fixes and Nix compatibility fixes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, seems sensible.

@jtojnar
Copy link
Member Author

jtojnar commented Mar 11, 2023

@ofborg eval

@ofborg ofborg bot requested a review from Twey March 11, 2023 16:19
@jtojnar jtojnar merged commit a6d96cc into NixOS:master Mar 23, 2023
@jtojnar jtojnar deleted the libbacktrace branch March 23, 2023 15:35
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