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

closure size: protobuf depends on C++ compiler #73919

Open
neongreen opened this issue Nov 22, 2019 · 15 comments
Open

closure size: protobuf depends on C++ compiler #73919

neongreen opened this issue Nov 22, 2019 · 15 comments
Labels
0.kind: bug 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: closure-size

Comments

@neongreen
Copy link

Describe the bug

Look at nixpkgs.protobuf:

$ nix-env -iA nixpkgs.protobuf
installing 'protobuf-3.7.1'
these paths will be fetched (2.14 MiB download, 17.78 MiB unpacked):
  /nix/store/mdybbfzbpdidmw95kj4r7z0cizrr3lfl-protobuf-3.7.1

How big is the closure? 700 MB

$ nix path-info -S /nix/store/mdybbfzbpdidmw95kj4r7z0cizrr3lfl-protobuf-3.7.1
/nix/store/mdybbfzbpdidmw95kj4r7z0cizrr3lfl-protobuf-3.7.1	  726819112

Why? Because it depends on clang (on macOS, at least)

$ nix why-depends /nix/store/mdybbfzbpdidmw95kj4r7z0cizrr3lfl-protobuf-3.7.1 /nix/store/h73a7xhf6l517dnk2sw0b4q64jbmaijn-clang-7.1.0

/nix/store/mdybbfzbpdidmw95kj4r7z0cizrr3lfl-protobuf-3.7.1
╚═══lib/libprotobuf.a: …:..................../nix/store/1vnw05l6bz30whmiang9i3156c064y7d
    => /nix/store/1vnw05l6bz30whmiang9i3156c064y7d-clang-wrapper-7.1.0
    ╚═══bin/clang: …k disable=SC2193.[[ "/nix/store/h73a7xhf6l517dnk2sw0b4q64jbmaijn-cla
        => /nix/store/h73a7xhf6l517dnk2sw0b4q64jbmaijn-clang-7.1.0

Metadata

 - system: `"x86_64-darwin"`
 - host os: `Darwin 18.7.0, macOS 10.14.6`
 - multi-user?: `no`
 - sandbox: `no`
 - version: `nix-env (Nix) 2.1.2`
 - channels(yom): `"nixpkgs-20.03pre200237.561b40dacbf"`
 - nixpkgs: `/Users/yom/.nix-defexpr/channels/nixpkgs`
@neongreen neongreen changed the title closure size: protobuf depends on C compiler closure size: protobuf depends on C++ compiler Nov 22, 2019
@neongreen
Copy link
Author

The problem is most likely with -g being passed to clang, not sure where it happens though.

@neongreen
Copy link
Author

My solution:

# https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/libraries/protobuf/generic-v3.nix

  postConfigure = ''
    sed -i -e 's/ -g / /g' Makefile src/Makefile gmock/make/Makefile googletest/make/Makefile
  '';

Seems hacky.

@veprbl
Copy link
Member

veprbl commented Nov 25, 2019

It seems like upstream also doesn't want to use "-g":
https://github.com/protocolbuffers/protobuf/blob/a9f390f44e3d138e1e94fd17046f865517b3b62d/configure.ac#L39-L44
Must be a bug somewhere?

@neongreen
Copy link
Author

I also don't get why strip doesn't strip away the strings -g adds.

@doronbehar
Copy link
Contributor

Hey all, on my system, I have protobuf in my store and:

$ nix-store -q --references /nix/store/n5h11njhx44x4hrl8zy4gbyq6z05i4sm-protobuf-3.7.1/ /nix/store/n687v0mblbkk0qi4lnw0vzbn01p5z2ln-protobuf-3.8.0/
/nix/store/an6bdv4phxsz14q2sk57iscl2dc7bnj1-glibc-2.30
/nix/store/jy89v2q2zv074mvw91jgqcvkmk7yqx69-gcc-9.3.0-lib
/nix/store/zsr1dksfh97yvb49k3rni00q5lppd9pd-zlib-1.2.11
/nix/store/n5h11njhx44x4hrl8zy4gbyq6z05i4sm-protobuf-3.7.1
/nix/store/n687v0mblbkk0qi4lnw0vzbn01p5z2ln-protobuf-3.8.0

gcc-lib is there but I don't think that's as bad as having clang there right?

@veprbl veprbl linked a pull request Apr 10, 2020 that will close this issue
10 tasks
@nh2
Copy link
Contributor

nh2 commented Apr 10, 2020

I would very much like to NOT disable -g builds, which would make dontStrip surprisingly not work.

I have had various protobuf related crashes, and using dontStrip helps to quickly investigate that in production (especially due to protobufs reliance on 32-bit ints that can silently overflow and corrupt data, see protocolbuffers/protobuf#5285 and google/proto-lens#249). Making dontStrip work only sometimes would be detrimental in my opinion.

Alternative suggestion:

Both the issue description and @doronbehar's post above suggest that this problem exists only on Darwin, and that on Linux the closure is not big. Could we make this change a Darwin-only workaroud?

@nh2
Copy link
Contributor

nh2 commented Apr 10, 2020

For completeness of linking issues, protocolbuffers/protobuf#6941 (comment) suggests that upstream is not quite sure which flags they want.

@veprbl veprbl linked a pull request Apr 10, 2020 that will close this issue
10 tasks
@neongreen
Copy link
Author

The last time I checked, on Linux it included gcc, which was pretty bad too (130 MB): juspay/fencer#31 (comment). Perhaps it's fixed now.

@neongreen
Copy link
Author

This said, "Making dontStrip work only sometimes" might be bad. Is there any other way to remove those -g-inserted paths without removing any other useful information added by -g?

@nh2
Copy link
Contributor

nh2 commented Apr 11, 2020

The last time I checked, on Linux it included gcc, which was pretty bad too (130 MB):

That seems to be fixed now:

$ nix path-info -Sh $(nix-store -q --references /nix/store/rmwda8bcsjaim9ng68siavp0scx9ipkv-protobuf-3.7.1)
/nix/store/5zzgkw8f7p4514bbmr018fyyyvpd46kp-gcc-9.2.0-lib 	  32.8M
/nix/store/a3q9zl42d0hmgwmgzwkxi5qd88055fh8-zlib-1.2.11   	  27.0M
/nix/store/aag9d1y4wcddzzrpfmfp9lcmc7skd7jk-glibc-2.27    	  26.8M
# together
/nix/store/rmwda8bcsjaim9ng68siavp0scx9ipkv-protobuf-3.7.1	  56.7M

Only depends on gcc-lib, not gcc.

@stale
Copy link

stale bot commented Oct 9, 2020

Hello, I'm a bot and I thank you in the name of the community for opening this issue.

To help our human contributors focus on the most-relevant reports, I check up on old issues to see if they're still relevant. This issue has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

The community would appreciate your effort in checking if the issue is still valid. If it isn't, please close it.

If the issue persists, and you'd like to remove the stale label, you simply need to leave a comment. Your comment can be as simple as "still important to me". If you'd like it to get more attention, you can ask for help by searching for maintainers and people that previously touched related code and @ mention them in a comment. You can use Git blame or GitHub's web interface on the relevant files to find them.

Lastly, you can always ask for help at our Discourse Forum or at #nixos' IRC channel.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 9, 2020
@nh2
Copy link
Contributor

nh2 commented Oct 10, 2020

Needs update from @neongreen whether based on the findings above, it'd be acceptable for him to make this change Darwin-only.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 10, 2020
@neongreen
Copy link
Author

I don't mind it being a Darwin-only workaround if it's not an issue on Linux anymore.

@doronbehar
Copy link
Contributor

It's not:

$ nix-store --query --references /nix/store/p4dcl742wd5hf0dkjqbzh4h8bjx7dx00-protobuf-3.13.0/
/nix/store/9df65igwjmf2wbw0gbrrgair6piqjgmi-glibc-2.31
/nix/store/9sfmwj09ij65qnc8dgv8h56gf12b60nn-zlib-1.2.11
/nix/store/vran8acwir59772hj4vscr7zribvp7l5-gcc-9.3.0-lib
/nix/store/p4dcl742wd5hf0dkjqbzh4h8bjx7dx00-protobuf-3.13.0

@stale
Copy link

stale bot commented Jun 7, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: closure-size
Projects
None yet
4 participants