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

Drop undocumened "feature" of destination-path for `--add-data` and `--add-binary`? #3066

Closed
htgoebel opened this Issue Dec 3, 2017 · 1 comment

Comments

Projects
None yet
2 participants
@htgoebel
Member

htgoebel commented Dec 3, 2017

Currently (v3.3), the destination-path --add-data and --add-binary as well as the Analysis() parameters data and binaries have an undocumented "feature" (*):

The destination-path (second element) can be an empty string, in which case the target directory's name is the source directory's basename.

I propose to remove this behavior. Instead, if an empty string is passed, PyInstaller should terminate with a useful message. E.g. "Empty target not allowed. Maybe you want to used %r" % os.curdir

Rational

  • The behavior is not documented in the manual and is not documented well in the code.
  • The behavior is not obvious.
  • This only works if the source is a directory or the source-glob only contains directories. As soon as
    the source is a file (of a glob resulting in a file), the empty string is not handled in the code and files
    will end up somewhere.
  • The same can be achieved easily by just adding the directory basename as destination.
  • This is a source of errors, see e.g. #3062 .

What do you think?

(*) To be more precise: The behavior of the empty string only documented in the code, but the manual does not mention it. Also the manual does not contain an example with an empty target path.

@tallforasmurf

This comment has been minimized.

Contributor

tallforasmurf commented Dec 3, 2017

FIne by me. Minor comments on the message. One, it should use the same term as in the docs, which is consistently "DEST" ("--add-binaries SRC:DEST"). Two, I do not understand suggesting os.curdir as a DEST argument. Would that be valid for a bundled app on another machine?

Another bit of tidy-up (maybe need a different issue#) would be to clarify the sections Adding Data Files and Adding Binary Files by using the terms "SRC" and "DEST" in those sections. That would more clearly associate them to the command syntax used earlier in the manual.

htgoebel added a commit to htgoebel/pyinstaller that referenced this issue Mar 24, 2018

building: util: Disallow empty DEST when adding binary or data files.
Instead, if an empty string is passed, PyInstaller will now terminate with a
useful message.

The destination-path `--add-data` and `--add-binary` had an undocumented
"feature": The destination-path (second element) could have been be an empty
string, in which case the target directory's name is the source directory's
basename.

The same was true for the `Analysis()` parameters `data` and `binaries`, but
here this was documented in the code - while not in the `Analysis()` API
documentation.

Rational

* The behavior is not documented in the manual and is not documented well in
  the code.

* The behavior is not obvious.

* This only works if the source is a directory or the source-glob only
  contains directories. As soon as the source is a file (of a glob resulting
  in a file), the empty string is not handled in the code and files will end
  up somewhere.

* The same can be achieved easily by just adding the directory basename as
  destination.

* This is a source of errors, see e.g. pyinstallergh-3062.

Closes pyinstallergh-3066.

@htgoebel htgoebel added this to the PyInstaller 3.4 milestone Mar 24, 2018

@htgoebel htgoebel closed this in 771986d Mar 26, 2018

zas added a commit to zas/picard that referenced this issue Sep 12, 2018

Fix pyinstaller complaining about empty DEST
It is enforced by pyinstaller 3.4

Empty DEST not allowed when adding binary and data files. Maybe you want to used '.'.
Caused by 'discid.dll'.

pyinstaller/pyinstaller#3066

@zas zas referenced this issue Sep 12, 2018

Merged

Fix pyinstaller complaining about empty DEST #966

1 of 5 tasks complete

mattura added a commit to mattura/volatility that referenced this issue Nov 6, 2018

Fix updated pyinstaller "empty DEST" issue
Current version of pyinstaller cannot accept an empty DESTINATION, assuming current directory.
pyinstaller/pyinstaller#3066.
This affects "hook-yara.py", "hook-distorm3.py" and "hook-openpyxl.py"

mattura added a commit to mattura/volatility that referenced this issue Nov 6, 2018

Fix updated pyinstaller "empty DEST" issue
Current version of pyinstaller cannot accept an empty DESTINATION, assuming current directory.
pyinstaller/pyinstaller#3066.
This affects "hook-yara.py", "hook-distorm3.py" and "hook-openpyxl.py"

mattura added a commit to mattura/volatility that referenced this issue Nov 6, 2018

Fix updated pyinstaller "empty DEST" issue
Current version of pyinstaller cannot accept an empty DESTINATION, assuming current directory.
pyinstaller/pyinstaller#3066.
This affects "hook-yara.py", "hook-distorm3.py" and "hook-openpyxl.py"

mattura added a commit to mattura/volatility that referenced this issue Nov 6, 2018

Fix updated pyinstaller "empty DEST" issue
Current version of pyinstaller cannot accept an empty DESTINATION, assuming current directory.
pyinstaller/pyinstaller#3066.
This affects "hook-yara.py", "hook-distorm3.py" and "hook-openpyxl.py"

mattura added a commit to mattura/volatility that referenced this issue Nov 6, 2018

Fix updated pyinstaller "empty DEST" issue
Current version of pyinstaller cannot accept an empty DESTINATION, assuming current directory.
pyinstaller/pyinstaller#3066.
This affects "hook-yara.py", "hook-distorm3.py" and "hook-openpyxl.py"

cowo78 pushed a commit to cowo78/pyinstaller that referenced this issue Dec 7, 2018

building: util: Disallow empty DEST when adding binary or data files.
Instead, if an empty string is passed, PyInstaller will now terminate with a
useful message.

The destination-path `--add-data` and `--add-binary` had an undocumented
"feature": The destination-path (second element) could have been be an empty
string, in which case the target directory's name is the source directory's
basename.

The same was true for the `Analysis()` parameters `data` and `binaries`, but
here this was documented in the code - while not in the `Analysis()` API
documentation.

Rational

* The behavior is not documented in the manual and is not documented well in
  the code.

* The behavior is not obvious.

* This only works if the source is a directory or the source-glob only
  contains directories. As soon as the source is a file (of a glob resulting
  in a file), the empty string is not handled in the code and files will end
  up somewhere.

* The same can be achieved easily by just adding the directory basename as
  destination.

* This is a source of errors, see e.g. pyinstallergh-3062.

Closes pyinstallergh-3066.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment