ruby-modules/bundler-env: Replace makeWrapper with makeBinaryWrapper to make bundled ruby apps work on macOS #161298

bergkvist · 2022-02-22T00:49:27Z

Fixes the following error on macOS when trying to run cewl:

$ nix-shell -p cewl --run cewl

/nix/store/zallj5aaf22ic1ic44sy4fpaiq1jvhij-cewl-5.5.2/bin/cewl: line 20: VERSION: command not found
/nix/store/zallj5aaf22ic1ic44sy4fpaiq1jvhij-cewl-5.5.2/bin/cewl: line 22: puts: command not found
/nix/store/zallj5aaf22ic1ic44sy4fpaiq1jvhij-cewl-5.5.2/bin/cewl: line 24: begin: command not found
/nix/store/zallj5aaf22ic1ic44sy4fpaiq1jvhij-cewl-5.5.2/bin/cewl: line 25: require: command not found
/nix/store/zallj5aaf22ic1ic44sy4fpaiq1jvhij-cewl-5.5.2/bin/cewl: line 26: require: command not found
/nix/store/zallj5aaf22ic1ic44sy4fpaiq1jvhij-cewl-5.5.2/bin/cewl: line 27: require: command not found
/nix/store/zallj5aaf22ic1ic44sy4fpaiq1jvhij-cewl-5.5.2/bin/cewl: line 28: require: command not found
/nix/store/zallj5aaf22ic1ic44sy4fpaiq1jvhij-cewl-5.5.2/bin/cewl: line 29: rescue: command not found
/nix/store/zallj5aaf22ic1ic44sy4fpaiq1jvhij-cewl-5.5.2/bin/cewl: line 31: syntax error near unexpected token `('
/nix/store/zallj5aaf22ic1ic44sy4fpaiq1jvhij-cewl-5.5.2/bin/cewl: line 31: `	if e.to_s =~ /cannot load such file -- (.*)/'

Likely also fixes other ruby programs depending on ruby-modules/bundler-env for macOS. According to nixpkgs-review, this seems to affect the following packages:

cewl discourse discourse gitaly gitlab gitlab-ee mastodon mikutter mpdcron redmine

Test it out:

env NIX_PATH=nixpkgs=https://github.com/bergkvist/nixpkgs/archive/macos-shebang.tar.gz \
nix-shell -p cewl --run "cewl --help"

Motivation for this change

nixpkgs uses bash wrapper scripts to modify environment variables that are used by executables. Sometimes these executables are interpreters, and placed in the shebang line of a script. On macOS, shebangs can't point to shell scripts, unlike on Linux.

pkgs.makeBinaryWrapper is a recent addition to nixpkgs, which is an alternative to pkgs.makeWrapper. It solves this particular problem by generating and compiling C code instead of generating a shell script. That means you can put it in a shebang line on macOS. The runtime overhead is also less than that of bash wrappers - with some disadvantages being a larger wrapper file size, and less features (no --run <shell-command> being the biggest one).

Relevant issues:

Things done

…to make bundled ruby apps work on macOS

NilsIrl · 2022-02-23T00:29:21Z

Result of nixpkgs-review pr 161298 run on aarch64-darwin 1

3 packages built:

cewl
mikutter
mpdcron

thiagokokada · 2022-03-08T14:29:27Z

This is definitively interesting but it seems to have some possible big implications (e.g.: all Ruby applications).

I can merge it but I definitively want someone else to review this.

@bergkvist Are you planning similar changes for other interpreted languages (e.g.: Python) 🤔 ?

thiagokokada · 2022-03-08T14:31:26Z

CC @roberth @SuperSandro2000 @doronbehar.

bergkvist · 2022-03-08T15:59:22Z

@thiagokokada Yes, I would like to change this for most interpreter wrappers in the future, including Python - so that the experience on macOS is significantly improved.

I was considering doing multiple interpreters in this PR initially, but decided to instead do this one at a time, since each such change will affect a lot of packages.

SuperSandro2000 · 2022-03-08T16:15:35Z

pkgs/development/ruby-modules/bundled-common/default.nix

@@ -118,7 +118,7 @@ let

      wrappedRuby = stdenv.mkDerivation {
        name = "wrapped-ruby-${pname'}";
-        nativeBuildInputs = [ makeWrapper ];
+        nativeBuildInputs = [ makeBinaryWrapper ];


Suggested change

nativeBuildInputs = [ makeBinaryWrapper ];

nativeBuildInputs = if stdenv.isDarwin [ makeBinaryWrapper ] else [ makeWrapper ];

I don't think we should convert all wrappers to binary ones just because Darwin is to limited to support basic things. If we would do it, debugging on Linux could become a bit of a pain sometimes. The wrapper includes the inputs but I don't want to think about the edge cases and possible breakages right now.

While I think that we should be cautious about this kinda of change to avoid breakage, I still think this has benefits for Linux systems too. For example, reducing the dependency tree to bootstrap (see #160323, where a failure to build bash on Android is causing freetype build to fail), and possible binaries that starts faster (see #124556 (comment) and #124556 (comment), yeah, this is macOS results but I also expect Linux to have some similar improvements).

I think BSD-systems also have the same shebang issue as macOS, without having FreeBSD/OpenBSD etc readily available to test this on.

By branching here on platform, if someone using Linux adds a --run-flag (or another flag which is supported by makeWrapper but not makeBinaryWrapper), they could break the wrappers on macOS without noticing and without breaking them on Linux.

By using the same wrapper type on both macOS and Linux, it means a change that causes a breakage on one platform will likely do so on other platforms as well - decreasing the chance that someone using Linux will break a package for macOS without noticing.

While I think that we should be cautious about this kinda of change to avoid breakage, I still think this has benefits for Linux systems too. For example, reducing the dependency tree to bootstrap (see #160323, where a failure to build bash on Android is causing freetype build to fail)

No, bash didn't fail to build there. It is most likely a splicing problem that a binary for a another platform is used and this shouldn't be changed by this PR. Also I think it is safe to assume that we cannot get bash out of bootstraping something. There is always some bash somewhere involved.

possible binaries that starts faster (see #124556 (comment) and #124556 (comment))

The performance improvement would be around ~3ms. Wrapping is very widely used and debugging failed shebangs is a pain to debug. I don't think the binary wrapper is so widely tested that it is safe to assume that it just works for everything. Though it might be safe to assume that it works for ruby things.

No, bash didn't fail to build there. It is most likely a splicing problem that a binary for a another platform is used and this shouldn't be changed by this PR. Also I think it is safe to assume that we cannot get bash out of bootstraping something. There is always some bash somewhere involved.

I am not saying to remove bash from boostraping, but having less things to depend on piece of code that is fairly complex is sure a plus.

The performance improvement would be around ~3ms. Wrapping is very widely used and debugging failed shebangs is a pain to debug. I don't think the binary wrapper is so widely tested that it is safe to assume that it just works for everything. Though it might be safe to assume that it works for ruby things.

The debugging part is fair enough, however don't assume that the performance improvement is useless. Of course, for things that run only one time it is insignificant, however for things that runs in loop this can add pretty quickly.

Also, of course the wrapper is not widely tested (actually right now it is not used at all in nixpkgs). But starting with ruby and them extending it to other usages once we get more testing is something that I think should start doing eventually.

By branching here on platform, if someone using Linux adds a --run-flag (or another flag which is supported by makeWrapper but not makeBinaryWrapper), they could break the wrappers on macOS without noticing and without breaking them on Linux.

Good point but that also means that we can't use the wrapper for everything yet.

Also this PR rebuilds more things than cewl.

Good point but that also means that we can't use the wrapper for everything yet.

Yeah, but nobody is saying that we are going to switch everything to use this wrapper right now.

Also this PR rebuilds more things than cewl.

The PR title is incorrectly but the commit message is correct(-ish).

I don't think it's such a pain to debug such wrappers. Here with Neovim, I opened for example ./results/cewl/bin/cewl and opened the binary wrapper interpreter from the shebang, and saw immediately the text in the binary contents:

# ------------------------------------------------------------------------------------ # The C-code for this binary wrapper has been generated using the following command: makeCWrapper /nix/store/vchcc0l18dj4pk07jv1z4qhd27lq0ni1-ruby-2.7.5/bin/ruby \ --set 'BUNDLE_GEMFILE' '/nix/store/9mbvhw5w5zm81ay0jf2jyx59g7a72zcr-gemfile-and-lockfile/Gemfile' \ --unset 'BUNDLE_PATH' \ --set 'BUNDLE_FROZEN' '1' \ --set 'GEM_HOME' '/nix/store/as2jwqvgcginmn13qzdqh5c8dyc27i6h-cewl-ruby-env/lib/ruby/gems/2.7.0' \ --set 'GEM_PATH' '/nix/store/as2jwqvgcginmn13qzdqh5c8dyc27i6h-cewl-ruby-env/lib/ruby/gems/2.7.0' # (Use `nix-shell -p makeBinaryWrapper` to get access to makeCWrapper in your shell) # ------------------------------------------------------------------------------------

So I don't feel in any way that this effects my wrappers debugging workflow, assuming I count on makeBinaryWrapper that this environment is actually set before the wrapper executes the final binary, and for that we have makeBinaryWrapper's golden tests.

The performance improvement would be around ~3ms.

Someone in a group that I participate confirmed those numbers on NixOS. A quick calculation here:

3ms * 1000 = 3s

3ms * 10000 = 30s

3ms * 100000 = 300s

I can definitively see some script calling a program multiple times in a loop hitting this number of calls, and eventually this becomes a very large overhead.

Yeah, maybe you could argue that a script calling a program this number of times is broken, and it should be converted to a program or etc, however I would argue that we should improve this if possible.

I think vim has word wrap on by default, so you only see it after executing set wrap!

doronbehar · 2022-03-08T17:03:04Z

Result of nixpkgs-review pr 161298 run on x86_64-linux 1

1 package failed to build:

mpdcron

9 packages built:

cewl
discourse
discourseAllPlugins
gitaly
gitlab
gitlab-ee
mastodon
mikutter
redmine

error: builder for '/nix/store/kz8xlf2rnd6iqkwhdxa6sq70ahspyb43-ruby2.7.5-nokogiri-1.10.3.drv' failed with exit code 1;
       last 10 log lines:
       >       |         ^~~~~~~~~~~~~
       > xml_document.c:511:3: note: in expansion of macro 'rb_scan_args'
       >   511 |   rb_scan_args(argc, argv, "03", &mode, &incl_ns, &with_comments);
       >       |   ^~~~~~~~~~~~
       > make: *** [Makefile:245: xml_document.o] Error 1
       >
       > make failed, exit code 2
       >
       > Gem files will remain installed in /nix/store/m73ppmzc7342kbvy9gnypq5h26iykgyn-ruby2.7.5-nokogiri-1.10.3/lib/ruby/gems/2.7.0/gems/nokogiri-1.10.3 for inspection.
       > Results logged to /nix/store/m73ppmzc7342kbvy9gnypq5h26iykgyn-ruby2.7.5-nokogiri-1.10.3/lib/ruby/gems/2.7.0/extensions/x86_64-linux/2.7.0/nokogiri-1.10.3/gem_make.out
       For full logs, run 'nix log /nix/store/kz8xlf2rnd6iqkwhdxa6sq70ahspyb43-ruby2.7.5-nokogiri-1.10.3.drv'.
error: 1 dependencies of derivation '/nix/store/bq06m84isag24530fyd1kxfgv95f1j1r-mpdcron-bundle.drv' failed to build
error: 1 dependencies of derivation '/nix/store/zch3gmprpvlcjkx05m0b0b882x7ip9pn-wrapped-ruby-mpdcron-bundle.drv' failed to build
error: 1 dependencies of derivation '/nix/store/66x3ylrxjxvraqgcwq6zp19xs014zsri-mpdcron-20161228.drv' failed to build

doronbehar · 2022-03-08T17:04:25Z

In general, we can merge such changes to a staging branch and use hydra builds to test that everything is functional. That might be a good idea when we'll want to make Python's wrappers binary, here perhaps it's not needed.

thiagokokada · 2022-03-08T17:08:50Z

In general, we can merge such changes to a staging branch and use hydra builds to test that everything is functional. That might be a good idea when we'll want to make Python's wrappers binary, here perhaps it's not needed.

I am quite in favor of using staging for Python, since it should affect much more packages than this one.

roberth

I understand the desire for stability and consistency, but the need for binary wrappers is a fact. If, worst case, we encounter a surprise, we'll learn something 🎉. +1 on this change.

On the topic of surprises, could you add a test? Even a basic test can rule out most packaging mistakes.

thiagokokada · 2022-03-09T13:32:43Z

I understand the desire for stability and consistency, but the need for binary wrappers is a fact. If, worst case, we encounter a surprise, we'll learn something 🎉. +1 on this change.

On the topic of surprises, could you add a test? Even a basic test can rule out most packaging mistakes.

makeBinaryWrapper has its own tests though. Do you want a test to be added to Ruby wrapped binary itself 🤔 ?

SuperSandro2000 · 2022-03-09T14:35:32Z

I am convinced now that we can slowly introduce binaryWrappers like this. They seem to be solid enough so far and I don't have a better idea to get them more tested and into the wild. If we encounter issues we just revert it and try to fix them.

Let me quickly add a basic test for wrappedRuby to make sure the binary still executes and push it and then we are good to go here.

thiagokokada · 2022-03-09T17:03:00Z

@roberth Can you re-review?

roberth · 2022-03-09T19:20:59Z

@ofborg test gitlab

ncfavier · 2022-03-14T21:05:37Z

I guess the people here will be interested in #164163

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ruby-modules/bundler-env: Replace makeWrapper with makeBinaryWrapper to make bundled ruby apps work on macOS #161298

ruby-modules/bundler-env: Replace makeWrapper with makeBinaryWrapper to make bundled ruby apps work on macOS #161298

bergkvist commented Feb 22, 2022 •

edited

Loading

NilsIrl commented Feb 23, 2022

thiagokokada commented Mar 8, 2022

thiagokokada commented Mar 8, 2022

bergkvist commented Mar 8, 2022

SuperSandro2000 Mar 8, 2022 •

edited

Loading

thiagokokada Mar 8, 2022 •

edited

Loading

bergkvist Mar 8, 2022 •

edited

Loading

SuperSandro2000 Mar 8, 2022

thiagokokada Mar 8, 2022

SuperSandro2000 Mar 8, 2022 •

edited

Loading

thiagokokada Mar 8, 2022

doronbehar Mar 8, 2022

thiagokokada Mar 9, 2022 •

edited

Loading

SuperSandro2000 Mar 9, 2022

doronbehar commented Mar 8, 2022 •

edited

Loading

doronbehar commented Mar 8, 2022

thiagokokada commented Mar 8, 2022

roberth left a comment

thiagokokada commented Mar 9, 2022

SuperSandro2000 commented Mar 9, 2022

thiagokokada commented Mar 9, 2022

roberth commented Mar 9, 2022

ncfavier commented Mar 14, 2022

amarshall commented Jan 26, 2023

	nativeBuildInputs = [ makeBinaryWrapper ];
	nativeBuildInputs = if stdenv.isDarwin [ makeBinaryWrapper ] else [ makeWrapper ];

ruby-modules/bundler-env: Replace makeWrapper with makeBinaryWrapper to make bundled ruby apps work on macOS #161298

ruby-modules/bundler-env: Replace makeWrapper with makeBinaryWrapper to make bundled ruby apps work on macOS #161298

Conversation

bergkvist commented Feb 22, 2022 • edited Loading

Motivation for this change

Things done

NilsIrl commented Feb 23, 2022

thiagokokada commented Mar 8, 2022

thiagokokada commented Mar 8, 2022

bergkvist commented Mar 8, 2022

SuperSandro2000 Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

thiagokokada Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

bergkvist Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

SuperSandro2000 Mar 8, 2022

Choose a reason for hiding this comment

thiagokokada Mar 8, 2022

Choose a reason for hiding this comment

SuperSandro2000 Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

thiagokokada Mar 8, 2022

Choose a reason for hiding this comment

doronbehar Mar 8, 2022

Choose a reason for hiding this comment

thiagokokada Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

SuperSandro2000 Mar 9, 2022

Choose a reason for hiding this comment

doronbehar commented Mar 8, 2022 • edited Loading

doronbehar commented Mar 8, 2022

thiagokokada commented Mar 8, 2022

roberth left a comment

Choose a reason for hiding this comment

thiagokokada commented Mar 9, 2022

SuperSandro2000 commented Mar 9, 2022

thiagokokada commented Mar 9, 2022

roberth commented Mar 9, 2022

ncfavier commented Mar 14, 2022

amarshall commented Jan 26, 2023

bergkvist commented Feb 22, 2022 •

edited

Loading

SuperSandro2000 Mar 8, 2022 •

edited

Loading

thiagokokada Mar 8, 2022 •

edited

Loading

bergkvist Mar 8, 2022 •

edited

Loading

SuperSandro2000 Mar 8, 2022 •

edited

Loading

thiagokokada Mar 9, 2022 •

edited

Loading

doronbehar commented Mar 8, 2022 •

edited

Loading