Skip to content
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

Fix environment #3243

Merged
merged 14 commits into from
Nov 19, 2018
Merged

Fix environment #3243

merged 14 commits into from
Nov 19, 2018

Conversation

frostming
Copy link
Contributor

@frostming frostming commented Nov 16, 2018

Signed-off-by: Frost Ming <mianghong@gmail.com>
@frostming frostming changed the title recognize editable packages as in project Fix bugs about editable packages Nov 16, 2018
@frostming frostming changed the title Fix bugs about editable packages [WIP] Fix bugs about editable packages Nov 16, 2018
Signed-off-by: Frost Ming <mianghong@gmail.com>
Signed-off-by: Frost Ming <mianghong@gmail.com>
@techalchemy
Copy link
Member

oh boy, this fix looks correct to me. Thanks for handling it

Signed-off-by: Frost Ming <mianghong@gmail.com>
@frostming frostming changed the title [WIP] Fix bugs about editable packages [WIP] Fix environment Nov 17, 2018
Signed-off-by: Frost Ming <mianghong@gmail.com>
Signed-off-by: Frost Ming <mianghong@gmail.com>
Signed-off-by: Frost Ming <mianghong@gmail.com>
Signed-off-by: Frost Ming <mianghong@gmail.com>
@frostming frostming changed the title [WIP] Fix environment Fix environment Nov 17, 2018
Signed-off-by: Frost Ming <mianghong@gmail.com>
Signed-off-by: Frost Ming <mianghong@gmail.com>
Signed-off-by: Frost Ming <mianghong@gmail.com>
@@ -276,6 +276,12 @@ def is_quiet(threshold=-1):
return PIPENV_VERBOSITY <= threshold


def is_in_virtualenv():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't like using globals everywhere to figure this out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the global environment variable is good and convenient but I need a way to get the right status when the environment is modified in runtime.(see the test case)

It isn't a good idea to change the logic only for testing though, because in real case the environment is always decided before any command is launched.

If you are not for this, I can revert these bits and consider using patching in test case.

with temp_environ():
c = delegator.run("virtualenv {}".format(virtualenv_path), block=True)
assert c.return_code == 0
for name in ("bin", "Scripts"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative fixture here is to use pipenv.environment.Environment() which provides a contextmanager called activated:

from pipenv.environment import Environment

@pytest.fixture()
def virtualenv(pathlib_tmpdir, PipenvInstance):
    virtualenv_path = pathlib_tmpdir / "venv"
    with temp_environ():
        c = delegator.run("virtualenv {}".format(virtualenv_path), block=True)
        assert c.return_code == 0
        environment = Environment(prefix=virtualenv_path.as_posix(), is_venv=True, pipfile=PipenvInstance.pipfile)
        with environment.activated():
            yield virtualenv_path

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.s. I have no idea if this works

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense for me.

Copy link
Member

@techalchemy techalchemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left you a small comment, not sure about the consequences of expanding script variables though, /cc @uranusjr

I'm ok with this if you want to move ahead with your fixture, just wanted to show an alternative

with temp_environ():
c = delegator.run("virtualenv {}".format(virtualenv_path), block=True)
assert c.return_code == 0
for name in ("bin", "Scripts"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.s. I have no idea if this works

@frostming frostming added this to the November Bugfix Release milestone Nov 19, 2018
@@ -2290,7 +2287,9 @@ def do_run_posix(script, command):
err=True,
)
sys.exit(1)
os.execl(command_path, command_path, *script.args)
os.execl(
command_path, command_path, *[os.path.expandvars(arg) for arg in script.args]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point I think os.execv (which takes a sequence directly) is better than execl.

@frostming
Copy link
Contributor Author

Great, let's move on. Thanks guys.

@frostming frostming merged commit 8de5a92 into master Nov 19, 2018
@frostming frostming deleted the editable-packages-fix branch November 19, 2018 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants