wrap-gapps-hook.sh: only wrap links when required #54909
Merged
+26
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mic92
reviewed
Jan 30, 2019
| find "${targetDirsThatExist[@]}" -type l -xtype f -executable -print0 \ | ||
| | while IFS= read -r -d '' linkPath; do | ||
| linkPathReal=$(realpath "${linkPath}") | ||
| for targetPath in "${targetDirsRealPath[@]}"; do |
There was a problem hiding this comment.
Making targetDirsRealPath an associative array would be preferable here to avoid
quadratic complexity.
if [[ -n ${targetDirsRealPath[$linkpath]} ]]; then
echo "Not wrapping link: '$linkPath' (already wrapped)"
continue
fiThere was a problem hiding this comment.
The alternative would be to memoize each wrapped programs once.
There was a problem hiding this comment.
@Mic92 Thanks for the review and for the nix-review program which proved very helpful in testing.
I'm not sure if an associative array will help here as the comparison at line 63 is intended to match any linkPathReal that starts with the prefix targetPath:
if [[ "$linkPathReal" == "$targetPath"* ]]; then
In the context of this script, targetDirsRealPath should have at most two elements, so performance should remain linear with respect to iterating over the symbolic links.
That said, your comment prompted me to review the code again and I think that there is a bug if neither /bin or /libexec exists in a package. I'll need to fix that!
|
@GrahamcOfBorg eval |
|
this needs to target staging |
tollb
force-pushed
the
fix/wrap-gapps-hook_unnecessary_symlink_wrap
branch
from
9dadac9 to
2ed1a3c
Compare
tollb
requested review from
basvandijk,
FRidh,
infinisil,
LnL7,
matthewbauer,
nbp,
peti,
ryantm and
ttuegel
as code owners
GrahamcOfBorg
added
6.topic: GNOME
GrahamcOfBorg
removed
6.topic: GNOME
jtojnar
reviewed
Jan 31, 2019
| continue 2 | ||
| fi | ||
| done | ||
| echo "Wrapping link: '$linkPath'" |
There was a problem hiding this comment.
@jtojnar Thanks for the review. I made the indentation fix.
jtojnar
removed request for
ryantm,
matthewbauer,
peti,
ttuegel,
basvandijk,
LnL7,
nbp,
FRidh and
infinisil
|
Other than that, LGTM. |
Unless dontWrapGapps is set, the wrap-gapps-hook.sh will currently wrap all executables (and symbolic links to executables) found under the target directories: bin and libexec. As a result, if a symbolic link in a target directory points to an executable in a target directory, both will get wrapped. This causes an extra shell/exec when following the symbolic link, as well as increasing the size of the final executable's environment. To avoid wrapping a link to an already wrapped executable, this commit splits the determination of what gets wrapped into two phases: 1. All binaries under the target directories are wrapped and logged with "Wrapping program ..." 2. All links to executables under the target directories are identified and checked to see if they reference an executable under one of the target directories. If yes, the required wrapping has already been performed on the associated binary (in phase 1), so no wrapping is done and "Not wrapping link: ... (already wrapped)" is logged. If no, the link points at an executable that hasn't been wrapped, so the link is wrapped and "Wrapping link: ..." is logged. As an example, the yelp package has a bin directory that contains an executable "yelp" and a symbolic link "gnome-help" -> "yelp". Prior to this commit, the bin directory would contain these files after wrapping: gnome-help -- wrapper to exec .gnome-help-wrapped .gnome-help-wrapped -- a symbolic link to yelp yelp -- wrapper to exec .yelp-wrapped .yelp-wrapped -- the original yelp binary After this commit, the bin directory will instead contain: gnome-help -- a symbolic link to yelp yelp -- wrapper to exec .yelp-wrapped .yelp-wrapped -- the original yelp binary NOTE: The primary motivation for this commit is to avoid obscuring the fact that two or more paths are simple aliases and expected to behave identically. It also reduces the likelihood of hitting limits related to environment variable size. LIMITATION: The method used above is intended to be conservative and will still wrap symbolic links to other symbolic links when the ultimate target is outside of bin or libexec.
tollb
force-pushed
the
fix/wrap-gapps-hook_unnecessary_symlink_wrap
branch
from
2ed1a3c to
bbb2f93
Compare
|
Thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unless dontWrapGapps is set, the wrap-gapps-hook.sh will currently
wrap all executables (and symbolic links to executables) found under
the target directories: bin and libexec.
As a result, if a symbolic link in a target directory points to an
executable in a target directory, both will get wrapped. This
causes an extra shell/exec when following the symbolic link,
as well as increasing the size of the final executable's environment.
To avoid wrapping a link to an already wrapped executable, this
commit splits the determination of what gets wrapped into two phases:
All binaries under the target directories are wrapped and logged
with "Wrapping program ..."
All links to executables under the target directories are
identified and checked to see if they reference an executable
under one of the target directories.
If yes, the required wrapping has already been performed on
the associated binary (in phase 1), so no wrapping is done
and "Not wrapping link: ... (already wrapped)" is logged.
If no, the link points at an executable that hasn't been
wrapped, so the link is wrapped and "Wrapping link: ..." is logged.
As an example, the yelp package has a bin directory that contains
an executable "yelp" and a symbolic link "gnome-help" -> "yelp".
Prior to this commit, the bin directory would contain these files
after wrapping:
After this commit, the bin directory will instead contain:
NOTE: The primary motivation for this commit is to avoid obscuring
the fact that two or more paths are simple aliases and expected to
behave identically. It also reduces the likelihood of hitting
limits related to environment variable size.
LIMITATION: The method used above is intended to be conservative
and will still wrap symbolic links to other symbolic links when
the ultimate target is outside of bin or libexec.
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)NOTE: Tested with nix-review. Used grep on logs to find packages with symbolic links that would no longer be wrapped after this commit. Here's a list:
You can build any of these packages to test this commit. Also, there is a default.nix that can be used to test link wrapping more comprehensively here:
https://gist.github.com/tollb/4a21b890058ca3ab7d44618e345d0e28