Bootloader: Ensure that symlinks to binaries do not get resolved.

[DavidVentura]

This fixes #3823

As far as I understand; the current behavior is wrong: when doing execvp() we are passing the resolved argv[0]. This means that the name under /proc/self/status will be incorrect.

If the new behavior is desired (it is for me at least; telegraf is reporting parent and child as different processes and that confuses my monitoring system) then the whole reading from /proc/ to get our full path is not needed anymore (as that will resolve all symlinks).

Let me know if you want me to change anything

[DavidVentura]

The current issue on appveyor seems unrelated. I added a second commit to make the builds work on windows but as far as I know, the new function should never be called from _WIN32.. so why is it being called/built?

[bjones1]

@DavidVentura, I agree. I looks like the failures originated with 6f9a912 per https://ci.appveyor.com/project/matysek/pyinstaller/builds/19649436. I'll open a separate issue for that.

[DavidVentura]

Great - Can you clarify why I needed to ifdef out my entire implmeentation? It is never going to be called on WIN32 platforms but yet it fails to build there anyway

[bjones1]

I would assume that realpath is a Linux-only function, so the Windows linker can't find it and the build fails. I would define it this way instead. Also, I think there's a bug in the code (see below).

#if !defined(_WIN32) && !defined(__APPLE__)
/*
 * Return full path to a file. Does not resolve symlinks.
 *
 * Returns false if the path could not be resolved; otherwise, the resolved path is place in ``abs``.
 */
int
pyi_path_fullpath_no_resolve(
    /* If ``abs`` is NULL, allocates and returns a new buffer which the caller
     * is responsible for freeing. Otherwise, ``abs`` should be a buffer of at
     * least PATH_MAX characters. */
    char *abs, 
    /* A relative path to be resolved. */
    const char *rel)
{
    char dirname[PATH_MAX];
    char full_dirname[PATH_MAX];
    char basename[PATH_MAX];
    pyi_path_basename(basename, rel);
    pyi_path_dirname(dirname, rel);
    if (realpath(dirname, full_dirname) == NULL) {
        return false;
    }
    /* ****** TODO ******: Shouldn't this be !=, so that a successful path join return true? */
    return (pyi_path_join(abs, full_dirname, basename) == NULL);
}
#endif

[htgoebel]

Thanks for the pull-request.

Some remarks prior to any code-review.

1. As implemented now, this breaks a rearly used and not well documented feature: The PYZ archive can be placed alongside with the executable instead of being attached to it, see pyi_main. The filename of the archive is based on executable.
   Of course, the archive should stimm be searched based on the resolved name of the executable.i If this is not done i pyi_path_executable, it needs to be done in pyi_path_archivefile.
   pyi_path_archivefile

2. Of course we need a test-case for your change. You can restrict it via @skipif_winorosx. I suggest to pytest.skip('... reasons ..') if /proc/$PID/status is missing (can be done in the test-function body, see test_pyz_as_external_file.

3. Of course we need a test-case for point (1). You can use test_pyz_as_external_file as a starting-point. To create the symlink, I suggest using a prepared spec-file which sets the symlink. Thus the test-case needs to be based on pyi_builder_spec instead of pyi_builder. Search the test-suite for pyi_builder_spec to learn how this can be done. You will need to pass app_name=<symlinked name> to pyi_builder_spec.test_spec(...) to make the test-site fetch the correct file.

Regarding the code: These #ifdef nested in a function (whcih are introduced in the second commit) are very, very ugly and hard to read. Please follow bjones1's suggestion in #3829 (comment).

You also need to submit a changelog entry so our users can learn about your change. (This is a new requirement since last September to speed up releasing.)

Thanks.

[DavidVentura]

I have just pushed a change with the changelog entry and proper ifdef - I do not see how this breaks current tests and I do not understand either

1. the desired behavior for pyz files
2. the current behavior for pyz files

Tests do not seem to be broken:

587 passed, 219 skipped, 37 xfailed, 94 warnings in 363.67 seconds

If there's a rarely used, not well documented and not tested feature.. is it really breaking?

I added a test that compares sys.argv[0][:15] to /proc/self/status's Name.. the code itself work if I test it manually, but I am not sure on how to get the test system to build it and run it; and then build it, symlink it and run that

[htgoebel]

Thanks for the adding the test. What I don't understand: What does it actually test? The script compares argv[0] and the name /proc/self/status. But I can't see where there is a symlink involved?!

As written above: You need to provide a spec-file, which creates a symlink after running EXE. Then you need another test-case, also using a spec-file creating a symlink, but passing append_pkg = False to EXE.

[DavidVentura]

Indeed, the test is what should be executed after I figure out how to get the symlinks to work..
How can I know where the executable is? Otherwise I don't know where to create the symlink.
Once I create the symlink, how do I get the bundle to execute it instead of the original script? Replacing name in COLLECT?

[DavidVentura]

I have created this broken spec file which creates a symlink in the appropiate directory pointing to the generated binary -- I am still unsure on how to get the tests to run the symlinked

lrwxrwxrwx 1 david dialout      18 Oct 28 00:46 qqqq -> spec-with-symlink1*
-rwxr-xr-x 1 david dialout 1212624 Oct 28 00:46 spec-with-symlink1*

[DavidVentura]

Ok, so I tried setting app_name to qqqq (the name I chose to create the test symlink) on test_spec but the problem is that it will look for all of the executables inside of APP_NAME that match the pattern APP_NAME - so not useful

------- Build finshed, now running executable. -------
# patterns in conftest's function _find_executables
/tmp/pytest-of-david/pytest-24/test_symlink_is_not_resolved0/dist/qqqq/qqqq
/tmp/pytest-of-david/pytest-24/test_symlink_is_not_resolved0/dist/qqqq
/tmp/pytest-of-david/pytest-24/test_symlink_is_not_resolved0/dist/qqqq/qqqq_?
/tmp/pytest-of-david/pytest-24/test_symlink_is_not_resolved0/dist/qqqq_?
# results
[]

[htgoebel]

Thanks so far! You made big progress and this is the way to go. Now we go into the not-well-documented corners of PyInstaller's test-suite:-) Looks like you already digged into this and I gave bad advise :-)

If you pass the normal app name (myapp) to both EXE and COLLECT and name the symlink link myapp_1, both the executable and the symlink should be picked up (as you show in #3829 (comment)). (For the test to work as expected, the app-name should be 10 characters max, since you write: "linux will truncate the name to 15 chars").

After you made this work, I'll do a detailed code-review.

[DavidVentura]

I just pushed these changes and indeed now they are being detected. I had to run execsnoop to find out if both the binary and the symlink were getting executed or not; how would you check for this ?

Also - As you can see I am creating 2 links: one in build/ and one in dist/ - the one in build was not being picked up by the test, but the one in dist did get picked up - why?

Is there a less hacky way for me to create the symlink in dist if it is still needed? To create it here I have to run it after COLLECT or otherwise dist doesn't exist yet..

[DavidVentura]

And now I added an xfail test to ensure that my code was actually doing something

[htgoebel]

Thanks for the update. Great to see it is working. I'll review the code whenI find some time for (this can become next week, so please be patient).

how would you check for this [if both the binary and the symlink were getting executed]?

Simply run pytest with -s to make it output stdout in any case. At ravis-ci stdout is only printed in case of an error.

Also - As you can see I am creating 2 links: one in build/ and one in dist/ - the one in build was not being picked up by the test, but the one in dist did get picked up - why?

Because the executable is searched in dist, and thus the symlink. dist contains the output defined be calling COLLECT. E.g. dynlibs are missing in build and only copied to dist. Since "symlink_1" is not passed to COLLECT. it os not copied to dist.

And now I added an xfail test to ensure that my code was actually doing something

This is not what xfail is meant for. xfail will continue it the test-case passes and only report at the end of the test-run. If some test-code should fail, then you should assert the code is actually failing.

Actually your second test-case doe not test anything usefull: The spec-file does not create a file called very_long_name_in_symlink, thus _test_executables() will always fail. Beside this I can see not much use in testing whether linux actually truncates the name to 15 characters. (If this behaviour would change, we should handle it in the test-script, not in the test-case.)

More remarks to come in the code review.

[DavidVentura]

Simply run pytest with -s to make it output stdout in any case. At ravis-ci stdout is only printed in case of an error.

thanks

Because the executable is searched in dist, and thus the symlink. dist contains the output defined be calling COLLECT. E.g. dynlibs are missing in build and only copied to dist. Since "symlink_1" is not passed to COLLECT. it os not copied to dist.

how can i pass the symlink to be copied over as well?

This is not what xfail is meant for.

How can i have a test that I want to fail?

Actually your second test-case doe not test anything usefull: The spec-file does not create a file called very_long_name_in_symlink, thus _test_executables()

Whoops - I meant for it to work so i will revisit it later

Beside this I can see not much use in testing whether linux actually truncates the name to 15 character

My point is not testing linux' behavior but rather ensure that if for some reason the implementation of the script were replaced by sys.exit(0) we would notice the tests are still working

[htgoebel]

I now found time for reviewing the code.Please see the in-line comments, most of which are nit-picking.
Also:

• Please remove the bootloader binaries. We only distribute bineries compilerd by one of the core developers.
• I wonder whether we should add more test-cases:
  • one where the symlink points to an absolute path (os.path.abspath(DISTPATH), untested). Maybe this could be put into the same test-case to speed up the test- suite. OTOH this makes it much more complicated to find out which of the cases failed. What do you think? Any idea?
  • one using a onefile build

... COLLECT ...

how can i pass the symlink to be copied over as well?

I was wrong here. It's much simpler to create the symlink in the DISTDIR than to tell COLLECT to copy a symlink.

This is not what xfail is meant for.

How can i have a test that I want to fail?

You want the test to pass :-) It's just that your test is "I want this to fail". See https://docs.pytest.org/en/latest/skipping.html?highlight=xfail for how xfail works. Thus you would need to wrap the pyi_builder_spec.test_spec(...) call by with pytest.raises(...)
see https://docs.pytest.org/en/latest/assert.html#assertions-about-expected-exceptions. Altough I cant't tell you ATM which exception to expect here.

Anyway I don't see any reason for test_symlink_is_not_resolved_fails_long.

My point is not testing linux' behavior but rather ensure that if for some reason the implementation of the script were replaced by sys.exit(0) we would notice the tests are still working

If this happens, the test-suite is broken and no longer reliable anyway. We would need to ensure this for all other tests, too. Thus I suggest to remove that test-case completely.

[htgoebel]

@DavidVentura I'd appreciate if you could finish this pull-request as I'm eager to merge it.

[htgoebel]

Trigger re-running the tests

[htgoebel]

@DavidVentura Thanks for this pull-request. I finally found time merging it. You could have had helped me on this a lot if you would have worked on my remarks.