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

stripDirs: Silence annoying 'File format not recognized' errors #23648

Merged
merged 1 commit into from Mar 9, 2017

Conversation

shlevy
Copy link
Member

@shlevy shlevy commented Mar 8, 2017

No description provided.

@mention-bot
Copy link

@shlevy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra and @peti to be potential reviewers.

@shlevy shlevy merged commit d39be63 into NixOS:staging Mar 9, 2017
@layus
Copy link
Member

layus commented Mar 9, 2017

@shlevy, @Mic92 : I am a bit annoyed about this PR. There has been work pending on this for some time, where the biggest difficulty was that it was not recommended to silence errors.
This just ignores errors from strip. I know that strip has inconsistent return values, but strip ... 2>/dev/null || true is too radical in a build. We need to detect OOM errors and such, which is done by parsing error messages, not ignoring them.
Please see #15087 and it's follow-up #15339.

@shlevy
Copy link
Member Author

shlevy commented Mar 9, 2017

@layus It's not possible for someone to know all of the pending work going on in nixpkgs. This fixes a real need (I have been asked about this by concerned users worried that their build failed 4 separate times in the three weeks I've been at my latest job, for example) and if/when #15339 is merged it can overwrite this change.

Are there any real cases of builds that OOM that would not have been detected if it weren't for the barrage of otherwise meaningless errors from the fixup phase?

@shlevy
Copy link
Member Author

shlevy commented Mar 9, 2017

Would be nice if github showed you other PRs that your PR would introduce merge conflicts for though.

@layus
Copy link
Member

layus commented Mar 9, 2017

@shlevy I understand the need for this change, and I even agree with the fix, until something better can be found. I tried to merge exactly the same patch several months ago, but it was refused for the reason cited above. Hence the more complex PR.

Are there any real cases of builds that OOM that would not have been detected if it weren't for the barrage of otherwise meaningless errors from the fixup phase?

See #5447

Would be nice if github showed you other PRs that your PR would introduce merge conflicts for though.

Yes, definitely.

I know this is the wrong place to rant about the ecosystem, as you are obviously not responsible for this situation. Seeing this being (auto) accepted so easily was a bit frustrating, knowing how much time I spent in the other alternative

Anyway, thanks for the improvement. This was badly needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants