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

apache: strip extra dirs. #14823

Closed
wants to merge 1 commit into from
Closed

Conversation

layus
Copy link
Member

@layus layus commented Apr 19, 2016

Apache keeps reference to build-time paths in /include files.
I bet these files could be simply removed, but creating a dev output works just as well.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @edolstra, @lethalman and @peti to be potential reviewers

@peti
Copy link
Member

peti commented Apr 19, 2016

There are a lot of seemingly unrelated changes in this PR?

@joachifm
Copy link
Contributor

joachifm commented Apr 19, 2016

Looks to me like what this PR actually does is change php to php.out and add apache modules to the list of stuff to strip (it already has a dev output). A mixup of some kind?

@layus layus force-pushed the closure-size-apache branch from 83b8783 to 7b56559 Compare April 19, 2016 12:26
@layus
Copy link
Member Author

layus commented Apr 19, 2016

Yes, indeed. I got my commits messed up.
Should be fixed.

@layus layus force-pushed the closure-size-apache branch from 7b56559 to ca03137 Compare April 19, 2016 12:30
@layus layus changed the title apache: add a dev output to remove build dependencies apache: strip extra dirs. Apr 19, 2016
@@ -23,6 +23,7 @@ stdenv.mkDerivation rec {
# FIXME: -dev depends on -doc
outputs = [ "dev" "out" "doc" ];
setOutputFlags = false; # it would move $out/modules, etc.
stripDebugList= [ "bin" "lib" "modules" ]; # also strip modules.
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this modification? This doesn't seem to change anything, really. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it does not change anything to apache 2.2, it simply unifies the syntax with 2.4.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could of course unify the other way around, but I feel this makes more sense with the name of the variable, and with the other pull requests I just made.

@layus
Copy link
Member Author

layus commented Apr 19, 2016

In fact, I am not sure this should be merged. Our stripping strategy should be smarter that to try only bin/ and lib/ and just strip any ELF in the derivation.
Adding extra directories to strip on a per-derivation basis is just calling for issues.

@edolstra
Copy link
Member

The only problem with stripping everything is that it will give a lot of pointless warnings about non-ELF files. That could be solved by using the isELF function, as the separate-debug-info.sh and patchelf setup hooks do.

@layus
Copy link
Member Author

layus commented Apr 19, 2016

@edolstra This seems something we should really do. Wild references are a pain to debug.
I will try to submit something fitting.

@dezgeg
Copy link
Contributor

dezgeg commented Apr 19, 2016

Do note though that there are other things than ELFs that presumably need stripping as well, such as .a archives and Darwin executables.

@layus
Copy link
Member Author

layus commented Apr 19, 2016

If error message are really an issue, why not strip everything and &>/dev/null the output ?

@dezgeg
Copy link
Contributor

dezgeg commented Apr 19, 2016

Also, #5447 is related.

@edolstra
Copy link
Member

Piping to /dev/null is not a good idea because it will hide actual error messages.

@dezgeg dezgeg added 0.kind: enhancement Add something new 6.topic: closure size The final size of a derivation, including its dependencies labels Apr 27, 2016
@peti
Copy link
Member

peti commented Jul 19, 2016

What is the state of this PR? Is it still relevant?

@layus
Copy link
Member Author

layus commented Jul 20, 2016

It is still relevant as the problem still exists. However, the actual fix is being developed in #15339. That fix is taking a while to get merged as it changes stdenv (modifies the strip phase).
To keep the PR list clean, I think we could close this issue in the meantime.

@layus layus closed this Jul 20, 2016
layus referenced this pull request Mar 22, 2017
We did this for 2.2 (cc61d31) but
lost this for 2.4. This reduces the Apache closure size from 312 MiB
to 102 MiB (primarily by getting rid of -dev outputs).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: enhancement Add something new 6.topic: closure size The final size of a derivation, including its dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants