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

Refactor IsolatedEnv, take two #537

Merged
merged 8 commits into from
Jan 25, 2023

Conversation

layday
Copy link
Member

@layday layday commented Nov 21, 2022

This is a more modest set of changes than I had in #361. Changing the runner's signature to make removing variables from the environ possible can come in a follow-up PR. Just as #361, this removes the env builder, redefines the isolated env interface, and adds an isolated env constructor to the project builder.

@layday layday mentioned this pull request Nov 21, 2022
@layday layday force-pushed the delegate-scripts-dir-to-isolated-env branch 3 times, most recently from 5e93049 to 827ac96 Compare November 21, 2022 16:20
@henryiii henryiii force-pushed the delegate-scripts-dir-to-isolated-env branch from 827ac96 to b345170 Compare December 2, 2022 17:03
@layday layday force-pushed the delegate-scripts-dir-to-isolated-env branch 2 times, most recently from e896a14 to 2c26411 Compare December 28, 2022 20:02
@layday
Copy link
Member Author

layday commented Dec 29, 2022

Is there something I can do to push this forward?

@henryiii
Copy link
Contributor

Just need to push a release, then this can be reviewed and merged. (Technically can be reviewed anytime, but at least I'm likely busy till after a release).

Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

A few minor questions and thoughts.

src/build/__init__.py Show resolved Hide resolved
src/build/__init__.py Show resolved Hide resolved
FailedProcessError,
TypoWarning,
)
from ._util import check_dependency, parse_wheel_filename
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a util, also having a _util seems confusing? If it's important to hide these, they could be _check_dependency and _parse_wheel_filename in util? Or maybe they could be in a file with a better private name?

Having public and private utils separated in this way isn't terrible though, just checking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suppose we could call it _helpers. If we underscore the functions themselves we can't import them without it being flagged as an error by Mypy and Pyright.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes they can, we just need __all__ (which we should be adding anyway, along with __dir__).

Copy link
Member Author

@layday layday Jan 19, 2023

Choose a reason for hiding this comment

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

But that would make them public assuming they are contained in a public (i.e. ununderscored) module. check_dependency is already public of course but parse_wheel_filename is private.

Copy link
Contributor

@henryiii henryiii Jan 19, 2023

Choose a reason for hiding this comment

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

I'd say that an underscored name in a __all__ is still hidden, it just means it is used from other modules. Using it downstream is just as risky as if there was no __all__. __all__ describes what is used by other modules - things not in __all__ should only be used in the file they reside in.

You could even filter things that start with an underscore in __dir__, but I'd not bother, things like IDEs and IPython still filter them by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are they also hidden from Sphinx?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think having a private module for internal use is cleaner FWIW, but I don't feel strongly about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with it either way. Just a note.

Sphinx hides things starting with an underscore by default, but it's configurable, IIRC.

src/build/__init__.py Show resolved Hide resolved
src/build/__init__.py Outdated Show resolved Hide resolved
src/build/__init__.py Show resolved Hide resolved
Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

@gaborbernat or @FFY00, could we get one more pair of eyes? If we don't hear anything, let's merge Jan 25.

- `install` is not required to interface with the backend --
  consumers can provision the isolated env in whichever manner
  they see fit.
- Assume responsibility for mutating `os.environ` -- different
  isolated envs may need to modify different env vars.
  As a consequence, the `scripts_dir` does not
  need to be exposed to the `ProjectBuilder`.
Reorganised the `ProjectBuilder` to accommodate the new constructor.
`scripts_dir` was removed and mutating the `python_executable` is
not supported anymore.  `srcdir` was renamed to `source_dir` to parallel
the hooks' `*_dir` params.
@layday layday force-pushed the delegate-scripts-dir-to-isolated-env branch from c741c1c to 28c95a7 Compare January 25, 2023 15:12
@layday layday merged commit f6fd696 into pypa:main Jan 25, 2023
@henryiii henryiii mentioned this pull request Feb 3, 2023
@layday layday deleted the delegate-scripts-dir-to-isolated-env branch March 1, 2024 18:42
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.

2 participants