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

Jump to bottom

stdenv: add isELFExec, isELFDyn, fix wrappers #66725

Merged
merged 2 commits into from Aug 18, 2019
Merged

stdenv: add isELFExec, isELFDyn, fix wrappers #66725

merged 2 commits into from Aug 18, 2019

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Aug 16, 2019

Motivation for this change

wrapQtAppsHook uses isELF from pkgs/stdenv/generic/setup.sh to determine whether to wrap a program or not.
This can accidentially wrap .so files (and other non-executables), so I added isELFExec and isELFDyn to be able to distinguish.
Afterwards, wrapQtAppsHook is changed to use isELFExec instead of just isELF.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: qt/kde 6.topic: stdenv Standard environment labels Aug 16, 2019
@flokli flokli mentioned this pull request Aug 16, 2019
Closed
@worldofpeace
Copy link
Contributor

I wonder if we should do similar for wrap-gapps-hook @jtojnar?

@matthewbauer
Copy link
Member

This seems alright, but it may take a while for this fix to go through staging (and might miss the release-19.09 cutoff). It would be good if we can get a fix for #65564 that avoids mass rebuilds, perhaps just inlining "isELFExec" in wrap-qt-apps-hook.sh would work?

@worldofpeace
Copy link
Contributor

This seems alright, but it may take a while for this fix to go through staging (and might miss the release-19.09 cutoff).

You think we couldn't merge staging more than two more times?
(I do have fixes in there that need to be in the release)

@worldofpeace
Copy link
Contributor

It would be good if we can get a fix for #65564 that avoids mass rebuilds, perhaps just inlining "isELFExec" in wrap-qt-apps-hook.sh would work?

I still think it will be a pretty large rebuild even if we do that.

@flokli
Copy link
Contributor Author

flokli commented Aug 16, 2019

We could probably backport something less intrusive if we really have to.
But I'd rather see the proper fix to go into staging ;-)

@matthewbauer
Copy link
Member

We could probably backport something less intrusive if we really have to.
But I'd rather see the proper fix to go into staging ;-)

Yeah that seems reasonable. My main concern is changing setup.sh means we have to rebuild everything, which can take up to a week, while just changing qt5-stuff is much quicker. On the other hand, it usually turns out that staging rebuilds everything anyway.

@flokli
Copy link
Contributor Author

flokli commented Aug 16, 2019

Checking for specific ELF types seemed very generic and useful outside of qt world too, like for the wrapGAppsHook, which is why I added it to stdenv setup.sh.

wrapGAppsHook currently seems to only check on the executable bit, so might run into similar issues if permission bits are a bit odd. I didn't check whether derivations set dontWrapGApps because of that - but this should probably be done in a follow-up PR, if we agree with this one.

@jtojnar
Copy link
Contributor

jtojnar commented Aug 16, 2019

Many GTK apps are written in scripting languages like Python so narrowing it down to ELF by default would not make sense. If we added a way to making it parametric with gappsMatchFile = "test -x" default, I guess that would be fine.

@jtojnar
Copy link
Contributor

jtojnar commented Aug 16, 2019

By the way, I could get rid of all the dontWrapGApps instances I introduced by extending the setup hook with a way to filter the matched files with something like gappsMatchFiles.

worldofpeace
worldofpeace suggested changes Aug 17, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Perl regex looks fine, standard output is ignored on grep correctly.
I've tested the bash functions by hand and they return the expected exit status.

Can we have comments like isELF has above the functions?

@flokli
Copy link
Contributor Author

flokli commented Aug 17, 2019

Uploaded a new iteration, now with comments.

I also updated the isELF implementation to be in grep too, as it doesn't make much sense for it to be implemented differently.

@edolstra
Copy link
Member

edolstra commented Aug 17, 2019 •
edited

The reason that isELF doesn't use grep is to avoid an expensive call to an external program. read etc. are all shell built-ins. This matters when you're iterating through thousands of files (calling grep is roughly 100x slower).

@flokli
Copy link
Contributor Author

flokli commented Aug 17, 2019

Makes sense, I'll skip changing isELF to grep then.

@flokli
setup.sh introduce isELFExec, isELFDyn
e1b80a5 
These can be used to determine whether a ELF file with ELF header is an
executable or shared library.

We can't implement it in pure bash, as bash has problems with null
bytes.
@flokli
wrapQtAppsHook: use isELFExec
88146a0 
This ensures we only wrap executables, not shared libraries
@flokli flokli merged commit dbd7ea5 into NixOS:staging Aug 18, 2019
@flokli flokli deleted the wrapqtappshook-exec branch August 18, 2019 11:58
@flokli flokli mentioned this pull request Aug 18, 2019
Merged
10 tasks
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixos-19-09-feature-freeze/3707/7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adisbladis adisbladis adisbladis left review comments

@Mic92 Mic92 Mic92 left review comments

@worldofpeace worldofpeace worldofpeace requested changes

@ttuegel ttuegel Awaiting requested review from ttuegel

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314 Ericson2314 is a code owner

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

Assignees
No one assigned
Labels
6.topic: qt/kde 6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@flokli @worldofpeace @matthewbauer @jtojnar @edolstra @nixos-discourse @adisbladis @Mic92