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

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

Merged
merged 2 commits into from
Mar 9, 2022
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e377a4d
ruby-modules/bundler-env: Replace makeWrapper with makeBinaryWrapper …
bergkvist Feb 22, 2022
a734cbb
wrappedRuby: add test for binaryWrapper
SuperSandro2000 Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions pkgs/development/ruby-modules/bundled-common/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{ stdenv, runCommand, ruby, lib, rsync
, defaultGemConfig, buildRubyGem, buildEnv
, makeWrapper
, makeBinaryWrapper
, bundler
}@defs:

Expand Down Expand Up @@ -118,9 +118,12 @@ let

wrappedRuby = stdenv.mkDerivation {
name = "wrapped-ruby-${pname'}";
nativeBuildInputs = [ makeWrapper ];
inherit (ruby) gemPath meta;
buildCommand = ''

nativeBuildInputs = [ makeBinaryWrapper ];
Copy link
Member

@SuperSandro2000 SuperSandro2000 Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@thiagokokada thiagokokada Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

@bergkvist bergkvist Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@SuperSandro2000 SuperSandro2000 Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@thiagokokada thiagokokada Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

image


dontUnpack = true;

buildPhase = ''
mkdir -p $out/bin
for i in ${ruby}/bin/*; do
makeWrapper "$i" $out/bin/$(basename "$i") \
Expand All @@ -131,6 +134,15 @@ let
--set GEM_PATH ${basicEnv}/${ruby.gemPath}
done
'';

dontInstall = true;

doCheck = true;
checkPhase = ''
$out/bin/ruby --help > /dev/null
'';

inherit (ruby) meta;
};

env = let
Expand Down