Increase resiliency of `runnable_binary` #1134

mishazharov · 2023-12-16T01:34:01Z

In the same vein as #1093, we want to allow runnable_binary to run on targets without shared libraries

Copy and pasted from #1093

When using the runnable_binary macro with a binary e.g. diff that does not involve any shared libraries, the following error is produced:

line 46: SHARED_LIBS_ARRAY[@]: unbound variable

As find . -name "*${SHARED_LIB_SUFFIX}" -print0 does not return any results (thus SHARED_LIBS_ARRAY) , along with set -uo earlier in the script, this assumes that there will always be some shared library files.

Also, this PR introduces checks for whether the directory which is specified by $RUNFILES_DIR actually exists. If it does not exist, cding into it will crash the script (even though the rest of the script would have run fine). This is applicable for cases where the binary has runfiles, and is also a runfile itself.

jsharpe · 2024-01-12T21:44:06Z

foreign_cc/private/runnable_binary_wrapper.sh

@@ -4,9 +4,20 @@
 # Otherwise, first cd to the runfiles dir for the wrapped executable before searching for shared libraries for the wrapped executable
 if [[ -z $RUN_UNDER_RUNFILES ]]; then
    SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
-    RUNFILES_DIR=${SCRIPT_DIR}/SH_BINARY_FILENAME.runfiles
+    RUNFILES_DIR_TMP=${SCRIPT_DIR}/SH_BINARY_FILENAME.runfiles


Under what conditions do the runfiles not exist? I have to admit I'm not familiar enough with the details of runfile layout to know if this is correct or not.

@fmeum is this logic correct or are we missing something else runfiles related here?

I think that this logic isn't sound, but it also wasn't sound before the PR (not entirely sure though, this is genuinely tricky). RUNFILES_DIR must always take precedence if already set, but this logic appears to overwrite it based on whether the current script has a runfiles directory, which can result in non-hermetic behavior.

Since the script uses the runfiles library below, could that setup code be moved up? That would provide the correct variables and this script wouldn't have to worry about the details. I also don't see a need to have a special case for --run_under.

Thank you for taking a look, I've just removed the special case related to --run_under. Since we don't use --run_under, I'm not sure if that preserves other users workflows (but it looks like they should continue working)

Under what conditions do the runfiles not exist?

I've found that using the previous logic would fail if the runnable binary was a data input to another binary. The rules_foreign_cc binary would be present in the runfiles directory, and wouldn't have a runfiles directory itself (since all the runfiles were aggregated in a top level directory)

That makes sense. If a binary is a data dependency of some other binary, the outer binary has to use the runfiles libraries to set RUNFILES_DIR, which the inner binary can then read and use.

mishazharov · 2024-01-24T06:49:44Z

@jsharpe @fmeum what do you all think about the new revision? A test is failing on windows because of a bad SSL certificate, but I think that's just because I busted the cache:

Downloading pcre2 patch from https://wrapdb.mesonbuild.com/v2/pcre2_10.40-3/get_patch
WARNING: failed to download with error: WrapDB connection failed to https://wrapdb.mesonbuild.com/v2/pcre2_10.40-3/get_patch with error <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:997)>.. Trying after a delay...
WARNING: failed to download with error: WrapDB connection failed to https://wrapdb.mesonbuild.com/v2/pcre2_10.40-3/get_patch with error <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:997)>.. Trying after a delay...
WARNING: failed to download with error: WrapDB connection failed to https://wrapdb.mesonbuild.com/v2/pcre2_10.40-3/get_patch with error <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:997)>.. Trying after a delay...
WARNING: failed to download with error: WrapDB connection failed to https://wrapdb.mesonbuild.com/v2/pcre2_10.40-3/get_patch with error <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:997)>.. Trying after a delay...
WARNING: failed to download with error: WrapDB connection failed to https://wrapdb.mesonbuild.com/v2/pcre2_10.40-3/get_patch with error <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:997)>.. Trying after a delay...
A fallback URL could be specified using patch_fallback_url key in the wrap file

(although I'm able to successfully download the patch locally, so I'm not too sure what's up there)

foreign_cc/private/runnable_binary_wrapper.sh

The second check can never be false if the first one is true

Mac uses a pretty old bash. So we: 1) don't use newer bash commands like -v 2) unconditionally declare all arrays 3) just check if they're empty instead

This commit removes the special case for `--run_under` which tried to find a runfile directory beside the currently executing script. This isn't needed because the runfiles bash script should set RUNFILES_DIR to the right thing anyway

foreign_cc/private/runnable_binary_wrapper.sh

jsharpe · 2024-02-01T20:20:49Z

CI failure seems to be a windows infrastructure issue with SSL certs so unrelated to this change.

mishazharov marked this pull request as draft December 16, 2023 01:40

mishazharov marked this pull request as ready for review December 16, 2023 02:03

mishazharov changed the title ~~Allow using runnable_binary on targets with no shared libraries~~ Increase resiliency of runnable_binary Dec 31, 2023

mishazharov force-pushed the runnable-binary-unset-vars branch from e36b6b2 to e0f03c9 Compare January 4, 2024 23:39

jsharpe reviewed Jan 12, 2024

View reviewed changes

snakethatlovesstaticlibs force-pushed the runnable-binary-unset-vars branch from 8aa5110 to 9ec9c91 Compare January 15, 2024 07:08

fmeum reviewed Jan 26, 2024

View reviewed changes

foreign_cc/private/runnable_binary_wrapper.sh Show resolved Hide resolved

foreign_cc/private/runnable_binary_wrapper.sh Show resolved Hide resolved

mishazharov added 11 commits February 1, 2024 00:55

Allow using runnable_binary on targets with no shared libraries

d6aee61

Try to get rid of warning via default expansion

7df69ca

Switch approach to checking if arrays are defined

805afd4

Get rid of second check, fix typos in docs

962ec61

The second check can never be false if the first one is true

Fix it to work on mac maybe

0fe9520

Mac uses a pretty old bash. So we: 1) don't use newer bash commands like -v 2) unconditionally declare all arrays 3) just check if they're empty instead

Ensure that RUNFILES_DIR exists before cd'ing into it

b6fb026

Fix typo

a89b93c

Do not set RUNFILES_DIR to non existent directory

0f175ac

Unconditionally cd into the previous directory

5041bb3

Remove special case for --run_under

a91d7ca

This commit removes the special case for `--run_under` which tried to find a runfile directory beside the currently executing script. This isn't needed because the runfiles bash script should set RUNFILES_DIR to the right thing anyway

Error on bad RUNFILES_DIR and set RUNFILES_DIR to abs path

dc6d1eb

snakethatlovesstaticlibs force-pushed the runnable-binary-unset-vars branch from 9ec9c91 to dc6d1eb Compare February 1, 2024 09:03

fmeum reviewed Feb 1, 2024

View reviewed changes

foreign_cc/private/runnable_binary_wrapper.sh Outdated Show resolved Hide resolved

mishazharov added 2 commits February 1, 2024 09:37

Use cross platform method of getting absolute path

a677af5

Fix abs path method

1b22e4f

fmeum approved these changes Feb 1, 2024

View reviewed changes

mishazharov requested a review from jsharpe February 1, 2024 19:54

jsharpe approved these changes Feb 1, 2024

View reviewed changes

jsharpe merged commit 4831827 into bazel-contrib:main Feb 1, 2024
2 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase resiliency of `runnable_binary` #1134

Increase resiliency of `runnable_binary` #1134

mishazharov commented Dec 16, 2023 •

edited

Loading

jsharpe Jan 12, 2024

fmeum Jan 13, 2024 •

edited

Loading

mishazharov Jan 15, 2024

fmeum Jan 15, 2024

fmeum Jan 15, 2024

mishazharov commented Jan 24, 2024

jsharpe commented Feb 1, 2024

Increase resiliency of runnable_binary #1134

Increase resiliency of runnable_binary #1134

Conversation

mishazharov commented Dec 16, 2023 • edited Loading

jsharpe Jan 12, 2024

Choose a reason for hiding this comment

fmeum Jan 13, 2024 • edited Loading

Choose a reason for hiding this comment

mishazharov Jan 15, 2024

Choose a reason for hiding this comment

fmeum Jan 15, 2024

Choose a reason for hiding this comment

fmeum Jan 15, 2024

Choose a reason for hiding this comment

mishazharov commented Jan 24, 2024

jsharpe commented Feb 1, 2024

Increase resiliency of `runnable_binary` #1134

Increase resiliency of `runnable_binary` #1134

mishazharov commented Dec 16, 2023 •

edited

Loading

fmeum Jan 13, 2024 •

edited

Loading