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

meson: 0.46.1 → 0.48.2 #46020

Merged
merged 9 commits into from Nov 18, 2018
Merged

meson: 0.46.1 → 0.48.2 #46020

merged 9 commits into from Nov 18, 2018

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Sep 3, 2018

Motivation for this change
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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@jtojnar
Copy link
Contributor Author

jtojnar commented Sep 3, 2018

@dtzWill I ran a GNOME session built with this and did not experience any problems. Tests are passing too. What issues did you encounter again?

@GrahamcOfBorg GrahamcOfBorg added 6.topic: GNOME GNOME desktop environment and its underlying platform 8.has: documentation 2.status: merge conflict labels Sep 3, 2018
@xeji xeji mentioned this pull request Sep 3, 2018
@dtzWill
Copy link
Member

dtzWill commented Sep 4, 2018

We patch(ed?) meson to change how it writes RPATH, which caused particularly painful (and not immediately obvious) results since apparently it works like this:

  • build using dummy rpath that's "surely" long enough
  • afterwards "blindly" replace dummy path with actual path, which is "known to be safe" since dummy was long enough.

It's a hack in the first place but we managed to very much break things by replacing paths of length ~30 ("dummy") with ones very much longer (> 200) which of course often resulted in invalid rpath searching but especially bad since those extra 170 chars or whatever overwrote whatever followed, which uhh hopefully wasn't important or something ;).

Bit more details here, although I was a bit battle-weary so didn't explain it very well:
#44040

As I recall, all the relevant parts of meson are the ones we already touch with our patches. I think. :).

@dtzWill
Copy link
Member

dtzWill commented Sep 4, 2018

In particular, everything was broken: #44040 (comment)

But it'd be good to ensure we're not generating corrupt binaries or at least be somewhat confident that's unlikely to happen....

@hedning
Copy link
Contributor

hedning commented Sep 7, 2018

We have a testing ground for meson now #45950, https://hydra.nixos.org/jobset/nixpkgs/gnome

Seems like things are working better than last time at least, I'm eg. able to run nixos.tests.login sucessfully. nixos.tests.systemd fails one of two tests, not sure if it's related though.

Doing this produces identical results, where nixpkgs.systemd is from master and ~/nixpkgs is the gnome branch with the meson update:

diff <(readelf -d $(nix eval nixpkgs.systemd --raw)/lib/libnss_resolve.so.2) \
     <(readelf -d $(nix eval -f ~/nixpkgs systemd --raw)/lib/libnss_resolve.so.2)

@dtzWill any tips on how to check if the rpath is wrong?

@jtojnar jtojnar mentioned this pull request Sep 10, 2018
9 tasks
@xeji xeji mentioned this pull request Sep 20, 2018
@jtojnar jtojnar changed the title meson: 0.46.1 → 0.47.2 meson: 0.46.1 → 0.48.0 Sep 25, 2018
@jtojnar jtojnar mentioned this pull request Sep 25, 2018
9 tasks
@jtojnar
Copy link
Contributor Author

jtojnar commented Sep 25, 2018

It looks like ./gir-fallback-path.patch is no longer needed?

$ rg shared-library $(nix-build -A atk.dev)/share/gir-1.0/Atk-1.0.gir
14:             shared-library="/nix/store/iiamyk9c1j8bzlxrf3lbz9g45xb8zlgd-atk-2.30.0/lib/libatk-1.0.so.0"
$ rg shared-library $(nix-build -A gst_all_1.gstreamer.dev)/share/gir-1.0/Gst-1.0.gir
16:             shared-library="/nix/store/drpcbmhq0znjn86k1xzij947kb247hr1-gstreamer-1.14.2/lib/libgstreamer-1.0.so.0"
$ rg shared-library $(nix-build -A gst_all_1.gstreamer.dev)/share/gir-1.0/Gst-1.0.gir
16:             shared-library="/nix/store/drpcbmhq0znjn86k1xzij947kb247hr1-gstreamer-1.14.2/lib/libgstreamer-1.0.so.0"

@jtojnar
Copy link
Contributor Author

jtojnar commented Oct 15, 2018

@dtzWill at #45950 (comment) wrote:

NixOS's extraUtils fails with this due to reference that's not allowed-- at first I feared this was the same problem as before (since the symptom is the same as when we were corrupting things re:meson), but I think it's unrelated :). Regardless I suspect you're running into this and if you haven't you probably will (or have fixed it and I'm on wrong branch :D).

In short the fix is adding -Dlink-udev-shared=false to systemd's mesonFlags. I didn't chase down why it suddenly started happening, but I can say I noticed in past few months various other distributions making changes regarding this so perhaps it's due to a meson upgrade they did before us :).

The harder fix, and untested, is that the extraUtils find-libs-and-copy-and-patchelf stuff is both too complicated and in particular fails to account for transitive dependencies (libraries that need other libraries). Well "fails" may be incorrect, this is possibly a feature and some would argue a case of underlinking but all I mean is this wouldn't happen if the xz dep was pulled in and patched to be used.

I think the solution proposed (meson flags) is the best as it prevents general systemd deps (== take over the world) from being pulled into our early boot process. We may want to do the same for systemctl (similar option) but that's not necessary so I'll leave that for another time.

Anyway thanks for your work, hope this helps, LMK if you have any questions or whatnot :).

I still cannot reproduce this failure when trying to build the following expression:

{pkgs ? import <nixpkgs> {} }:
let
  config.boot.loader.supportsInitrdSecrets = false;
  config.boot.isContainer = false;
  config.boot.initrd.secrets = {};
  config.boot.initrd.extraUtilsCommands = "";
  config.boot.initrd.extraUtilsCommandsTest = "";
  config.system.build.fileSystems = [];
  config.systemd.package = pkgs.systemd;
  inherit (pkgs) lib;

  utils = import <nixpkgs/nixos/lib/utils.nix> pkgs;
  stage-1 = import <nixpkgs/nixos/modules/system/boot/stage-1.nix> { inherit pkgs lib utils config; };

in stage-1.config.content.system.build.extraUtils

@dtzWill
Copy link
Member

dtzWill commented Oct 17, 2018

I still cannot reproduce this failure when trying to build the following expression:

Using that expression fails for me building against the gnome PR (where I commented) and when setting NIX_PATH appropriately, I'll try it against this PR shortly.

@jtojnar
Copy link
Contributor Author

jtojnar commented Nov 10, 2018

The current solution already should not leave any partial references since it replaces the whole length of the RPATH entry with NULL bytes before writing the new_rpath. Keeping the trailing part of old_rpath will not help with the potential Xorg issue because it will most definitely be stripped (the paths at the end are the ones added by ld-wrapper). I do not see how is the proposed solution any better than the current one.

@hedning
Copy link
Contributor

hedning commented Nov 10, 2018

Yeah, you're probably right. We're most likely already violating the assumptions made by meson when we're moving the stuff added by ld-wrapper to the start. So I don't really have any problems with going with the current solution. Just saw that you've already pushed the fix to the the gnome branch 👍

@jtojnar
Copy link
Contributor Author

jtojnar commented Nov 10, 2018

I cannot reproduce the issue at the moment.

This was referenced Nov 10, 2018
@jtojnar
Copy link
Contributor Author

jtojnar commented Nov 11, 2018

Based on #45950 (comment), our gir-fallback-path.patch is apparently still needed. The old patch is not really trivially portable, since our --fallback-library-path hack does not expect multiple libraries that are now supported.

@jtojnar jtojnar force-pushed the meson-0.47 branch 2 times, most recently from 7879b86 to 25aaecf Compare November 11, 2018 17:27
@jtojnar
Copy link
Contributor Author

jtojnar commented Nov 11, 2018

Re-added the patch.

Meson no longer propagates it so we need to re-add it.
Meson no longer propagates it so we need to re-add it.
Meson no longer propagates it so we need to re-add it.
Meson no longer propagates it so we need to re-add it.
Meson no longer propagates it so we need to re-add it.
Meson no longer propagates it so we need to re-add it.
Meson no longer propagates it so we need to re-add it.
@coreyoconnor
Copy link
Contributor

FYI: I also ran into the is not allowed to refer to error previously. I'm unable to reproduce the issue with the latest commits.

@jtojnar jtojnar changed the title meson: 0.46.1 → 0.48.1 meson: 0.46.1 → 0.48.2 Nov 17, 2018
@jtojnar jtojnar changed the base branch from master to staging November 17, 2018 18:16
@hedning
Copy link
Contributor

hedning commented Nov 18, 2018

(from irc) @jtojnar > hedning_: I am suspicious because you said you reproduce it after I pushed the current fix

Ah, sorry for the confusion, I should've made it clear that I was talking about the branch without the clear old_rpath fix. I was working under the assumption that the fix wasn't safe, but when reading #46020 (comment) again I can see I didn't specify that at all :(

@jtojnar
Copy link
Contributor Author

jtojnar commented Nov 18, 2018

Let's merge this and see what happens.

@jtojnar jtojnar merged commit 85bd2a7 into NixOS:staging Nov 18, 2018
GNOME automation moved this from In Progress to Done Nov 18, 2018
@jtojnar jtojnar deleted the meson-0.47 branch November 18, 2018 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
GNOME
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

9 participants