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

Patch for setup.py egg_info issue #5760

Merged
merged 14 commits into from
Jul 1, 2023
Merged

Patch for setup.py egg_info issue #5760

merged 14 commits into from
Jul 1, 2023

Conversation

matteius
Copy link
Member

@matteius matteius commented Jun 30, 2023

It was discovered that requirementslib uses the sys.executable python rather than the pipenv environment python. Which as @oz123 reminded me should be the same, but due to some weird code paths, it wasn't always the same in my testing when the resolver got invoked as subprocess.

The issue

Fixes #5753

The fix

I don't love the use of the environment variable, but I figured out this was the original intention of how the pipenv <--> requirements lib interface and at some point it got hard-coded to sys.executable in the resolver code which was getting invoked with a different python version at points.

This fixes the problem by ensuring that the correct python of the pipenv environment is being used.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix.rst, .feature.rst, .behavior.rst, .doc.rst. .vendor.rst. or .trivial.rst (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

…here requirementslib is using system python (until better patch can be made).
@matteius matteius closed this Jun 30, 2023
@matteius matteius deleted the issue-5753-patch branch June 30, 2023 01:02
@matteius matteius restored the issue-5753-patch branch June 30, 2023 03:36
@matteius matteius reopened this Jun 30, 2023
@matteius matteius marked this pull request as ready for review June 30, 2023 03:54
@matteius matteius requested a review from oz123 June 30, 2023 03:56
@matteius matteius changed the title Possible temporary patch for setup.py egg_info issue Patch for setup.py egg_info issue Jun 30, 2023
Comment on lines 1145 to 1146
stdout=sp.PIPE,
stderr=sp.PIPE,
Copy link
Contributor

@cclauss cclauss Jun 30, 2023

Choose a reason for hiding this comment

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

Suggested change
stdout=sp.PIPE,
stderr=sp.PIPE,
capture_output=True

% MY_CODE="import subprocess as sp ; sp.run([python, "setup.py"] + args, cwd=target_cwd, stdout=sp.PIPE, stderr=sp.PIPE)"

% echo $MY_CODE | ruff --select=UP022 --fix -

import subprocess as sp ; sp.run([python, setup.py] + args, cwd=target_cwd, capture_output=True)

% ruff rule UP022 # https://beta.ruff.rs/docs/rules/#pyupgrade-up

replace-stdout-stderr (UP022)

Derived from the pyupgrade linter.

Autofix is always available.

What it does

Checks for uses of subprocess.run that send stdout and stderr to a
pipe.

Why is this bad?

As of Python 3.7, subprocess.run has a capture_output keyword argument
that can be set to True to capture stdout and stderr outputs. This is
equivalent to setting stdout and stderr to subprocess.PIPE, but is
more explicit and readable.

Example

import subprocess

subprocess.run(["foo"], stdout=subprocess.PIPE, stderr=subprocess.PIPE)

Use instead:

import subprocess

subprocess.run(["foo"], capture_output=True)

References

Copy link
Contributor

Choose a reason for hiding this comment

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

Was the removal of cwd=target_cwd intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cclauss intentional, as it seemed unnecessary with the cd(target_cwd) context manager -- should already be in that directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well its finally having a good run -- some just turned green.

@matteius matteius merged commit ee20f40 into main Jul 1, 2023
@matteius matteius deleted the issue-5753-patch branch July 1, 2023 05:42
@cclauss
Copy link
Contributor

cclauss commented Jul 1, 2023

Thanks massively. This fixed the problem for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release v2023.6.26: error: invalid command 'egg_info'
2 participants