-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
WIP: add options to preserve environment variables #4401
base: develop
Are you sure you want to change the base?
WIP: add options to preserve environment variables #4401
Conversation
even though we have enough reason to delete VIRTUAL_ENV in environment, we should still keep the original data for those who relies on it essentially, provide another way to access it when necessary.
The newest commit contains the more generic solution about environment preserving, please check. I'm still trying to figure out how to add tests about this part, will update when I finish that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now found time to have a deeper look into the code. See my inline comments.
Anyway: This pull-request should either be "add renamed env var to replace VIRTUAL_ENV" or something like "Add option to preserve some environment variables."
Please mind to submit a changelog entry so our users can learn about your change. And this is why this pull-request should be either "add renamed env var …" or "Add option to preserve …", since the news-fragment can only ba about one change.
- add arg --pyiboot01-preserved-envs with default value VIRTUAL_ENV An additional pyiboot01_bootstrap.py file will be generated to `workpath` during build process, which contains the modified version of the source. This mechanism provides fine granular control over what pyiboot01 does when running the app, more options (template vars) could be added in the future if similar feature is requested. - add arg --pyiboot01-no-preserved-envs to control --pyiboot01-preserved-envs availability If the user does not want any env vars to be preserved, passing this argument will simply turn off this feature
a93cd05
to
2de522c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. I added more inline comments.
Of course we also need
- documentation - IMHO this option is worth an explanation section in the manual (I suggest
doc/runtime-information.rst
) - a test-case
The test-case should contain twotest_source()
calls, like this
# verifying the command line option actually works
pyi_builder.test_source(
"""
import os
assert os.genenv("PRESERVED…")
""",
pyi_args=['--preserve-…])
)
# verify the command-line option is not leaking env-vars to other builds
pyi_builder.test_source(
"""
import os
assert not os.genenv("PRESERVED…")
""")
PyInstaller/building/makespec.py
Outdated
metavar="ENVVAR", dest='pyiboot01_preserved_envs', | ||
help='Environment variables that will be preserved in pyiboot01_bootstrap module, ' | ||
'the preserved env var will be renamed with prefix `PYINSTALLER_PRESERVED_`.' | ||
'This option can be used multiple times. (default: VIRTUAL_ENV)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Option should be just
--preserve-envvar
. We must not bother users withpyiboot01
, which is an implementation detail. Also "pyiboot01_bootstrap module" should not occur in the help-message. -
Also this should be singular, since the option adds a single variable to the list.
-
I'd not add a default here. Most applications do not even
VIRTUAL_ENV
. Also be can omit the--no-…
option. -
"Environment variable" (singular!) "to be preserved when running the application".
-
"preserve" might not be the proper verb at all. I'm not a native speaker, but "retain", "copy", "keep" might be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally decided to call it --disguise-envvar
, as a verb-object phrase, I found the reason why I want to use the word "preserve" is because I only thought about the result of the env vars, they are indeed preserved, but our purpose is to hide them, or disguise, which has the meaning of change things to a different form, fits better in this situation.
Here's the LDOCE thesaurus of the two words:
- hide: to make something difficult to see or find, or to not show your true feelings
- disguise: to make someone or something seem like a different person or thing, so that other people cannot recognize them
I'm not a native speaker either, I agree we should choose the word very cautiously, glad to change it to other word if we can find out anything better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd not add a default here. Most applications do not even VIRTUAL_ENV. Also be can omit the --no-… option.
I actually don't want to add the default value either, but I thought it might be friendly to not change the default behavior while adding new features, if you think it's OK, then I'd be happy to remove it and the --no...
option (they were already removed in the latest code!).
Just added the news file and updated the title of this PR, forgive me for not reading the contribution guide before pushing the code, got too hurry in a time-limited situation 😢 |
PyInstaller/depend/analysis.py
Outdated
# type:(str, dict) -> str | ||
for k, v in context.items(): | ||
content = re.sub(r"'\{\{ " + k + r" \}\}'", repr(v), content) | ||
return content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest re.sub(re.escape("'{{%s}}'" % k), …
, which is easier to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea: What about not modifing a copy of pyiboot01_bootstrap, but to generate a new module e.g. pyimod04_variables
, which currently only defines PRESERVED_ENVS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest re.sub(re.escape("'{{%s}}'" % k), …, which is easier to read
Update as suggested
What about not modifing a copy of pyiboot01_bootstrap, but to generate a new module e.g. pyimod04_variables, which currently only defines PRESERVED_ENVS?
Considering that a new file adds more overhead, and a generated new file is more or less the same thing with a modified existing file (which means we won't do anything less in the code), I think using pyiboot01_bootstrap is enough. Also manipulating environment is as dangerous and important as updating sys module, they should sit together as company :D
Looks like we are working on this the same tim :-) I'll stop reviewing for new, waiting for your code to be settled a bit :-) |
also rename the dest arg name pyiboot01_no_preserved_envs to disguised_envs
new function is called `build_from_template`. other changes: - use re.escape for better readability - add unit test for build_from_template_file
The code has been updated for all the requests in the review, see the latest 4 commits and replies in the conversations. |
Almost forget this one, shall we pick it up and start over? |
3a8eccd
to
e20e74c
Compare
add arg --pyiboot01-preserved-envs with default value VIRTUAL_ENV
An additional pyiboot01_bootstrap.py file will be generated to
workpath
during build process, which contains the modified version of the source.
This mechanism provides fine-granular control over what pyiboot01 does
when running the app, more options (template vars) could be added in the future
if similar feature is requested.
add arg --pyiboot01-no-preserved-envs to control --pyiboot01-preserved-envs availability
If the user does not want any env vars to be preserved, passing this argument will simply
turn off this feature