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

protobuf: do a release build (without -g) #84867

Closed
wants to merge 1 commit into from
Closed

protobuf: do a release build (without -g) #84867

wants to merge 1 commit into from

Conversation

veprbl
Copy link
Member

@veprbl veprbl commented Apr 10, 2020 •
edited

This removes parts of the compiler from the closure.

Motivation for this change

Fixes: #73919

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
    ** on darwin:***
# nix path-info -Sh $(readlink before)
/nix/store/5vm6cpfhmcdnrp69shllns4qqij213y4-protobuf-3.7.1       693.1M
# nix path-info -Sh $(readlink after)
/nix/store/jd6igaiad4qzjb6salcq7pjgixk2jl2c-protobuf-3.7.1        59.8M

@veprbl
protobuf: do a release build (without -g)
18bce0e 
This removes parts of the compiler from the closure.
@veprbl veprbl added this to WIP in Staging via automation Apr 10, 2020
@veprbl veprbl moved this from WIP to Needs review in Staging Apr 10, 2020
@veprbl veprbl linked an issue Apr 10, 2020 that may be closed by this pull request
closure size: protobuf depends on C++ compiler #73919
Open
@nh2 nh2 mentioned this pull request Apr 10, 2020
Closed
10 tasks
@nh2
Copy link
Contributor

nh2 commented Apr 10, 2020 •
edited by FRidh

FYI there's another protobuf PR open currently that wants to switch to CMake: #84824

@nh2
Copy link
Contributor

nh2 commented Apr 10, 2020

I am not in favour of removing -g on Linux, but instead would prefer to make this a Darwin-only workaround -- details in #73919 (comment).

@veprbl
Copy link
Member Author

veprbl commented Apr 10, 2020

This could be made conditional on $dontStrip

@veprbl
Copy link
Member Author

veprbl commented Jun 24, 2020

@GrahamcOfBorg eval

nh2
nh2 requested changes Jun 25, 2020
View reviewed changes
Copy link
Contributor

@nh2 nh2 left a comment

Choose a reason for hiding this comment

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

As there's new activity on this PR, I'll make my request explicit using Github's review functionionality as usual (not sure why I didn't do it originally):

This could be made conditional on $dontStrip

I think we should do that, and I also think we should make it dontStrip on OSX only if the findings from #73919 (comment) are still true.

@SuperSandro2000 SuperSandro2000 marked this pull request as draft November 28, 2020 23:50
@veprbl veprbl closed this Dec 1, 2020
@veprbl veprbl deleted the pr/protobuf_no_g branch December 1, 2020 17:00
Staging automation moved this from Needs review to Done Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nh2 nh2 nh2 requested changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 101-500 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+
Projects
Staging
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

closure size: protobuf depends on C++ compiler
2 participants
@veprbl @nh2