Skip to content

Add PytestPluginSetup.extra_sys_path for pants-plugins#21274

Merged
cognifloyd merged 4 commits intomainfrom
cognifloyd/pytest-setup-pythonpath
Aug 11, 2024
Merged

Add PytestPluginSetup.extra_sys_path for pants-plugins#21274
cognifloyd merged 4 commits intomainfrom
cognifloyd/pytest-setup-pythonpath

Conversation

@cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Aug 5, 2024

This feature allows plugins to inject additional paths in the PEX_EXTRA_SYS_PATH env var, similar to how a wrapper script could modify PYTHONPATH when running pytest.

This will be helpful for plugins that need virtual "source roots" without explicitly registering them. A codegen plugin could create a new "source root" for example, or a python framework could have special PYTHONPATH requirements that do not map well to "source roots", especially if the roots overlap.

Background:

I need this for the StackStorm project where I have to deal with "StackStorm Content Packs" with special PYTHONPATH rules. These packs work best as a single source root, as I have a single pack_metadata(...) target that defaults to a recursive glob inclusion of yaml files (aka "metadata" files) in the pack. However, I also have python files that live in several directories that, by convention, get added to PYTHONPATH. If I try to create a source root for every one of those special directories, I get an explosion of source roots, and I would have to redesign pack_metadata(...) such that I have to add details about these yaml files in many more BUILD files. So, I don't want to expose those directories as source roots.

To facilitate this I have local changes to StackStorm's pack_metadata pants-plugin that record these special modules in the python module mapper (a FirstPartyPythonMappingImplMarker -> FirstPartyPythonMappingImpl rule) allowing dependency inference to work correctly for imports that rely on these special directories being on PYTHONPATH. I also extended the plugin to generate the relevant PYTHONPATH bits, so I just need the feature in this PR (or something similar) to allow me to actually include them when running pytest.

Alternatives

I could add a generic extra_env to PytestPluginSetup, and then set PytestPluginSetup(extra_env={"PEX_EXTRA_SYS_PATH": ":".join(my_extra_sys_path)}), but that would be more complex as it would have to merge the PEX_EXTRA_SYS_PATH that pants already generates, and possibly protect any other env vars injected from subsystem config or from particular targets. I don't actually need to modify any other env vars, so I opted to only implement extra_sys_path.

I also considered something more generic (not pytest-specific) where plugins can say "I have source roots as well", but then I run into the issue with overlapping source roots. Plus, that would mean potentially changing many of the call sites that request source roots. Something more generic, like this, would be nice for injecting the sys.path entries for other tools like pylint or goals like run and repl, but I don't see any obvious interfaces for a generic feature like this.

This feature allows plugins to inject additional paths in the
PEX_EXTRA_SYS_PATH env var, similar to how a wrapper script could modify
PYTHONPATH when running pytest.

This will be helpful for plugins that need virtual "source roots"
without explicitly registering them. A codegen plugin could create a new
"source root" for example, or a python framework could have special
PYTHONPATH requirements that do not map well to "source roots", esp if
the roots overlap.
@cognifloyd cognifloyd added category:plugin api change backend: Python Python backend-related issues labels Aug 5, 2024
@cognifloyd cognifloyd requested review from a team and benjyw August 5, 2024 22:08
@cognifloyd cognifloyd self-assigned this Aug 5, 2024
@cognifloyd
Copy link
Member Author

I'm not sure if this should be category:plugin api change or category:new feature as this new feature is not directly usable without writing a pants-plugin to use it. I'm happy to switch the category either way.

@cognifloyd cognifloyd requested review from kaos and tdyas August 6, 2024 13:02
@cognifloyd
Copy link
Member Author

@cognifloyd
Copy link
Member Author

Here are the locations that could also use the additional PYTHONPATH paths. Unlike pytest there is not a readily available plugin hook I can use to inject this.

Tools:

Goals:

  • run:
    source_roots = [
    *([] if run_in_sandbox else sources.source_roots),
    *chrooted_source_roots,
    ]
    extra_env = {
    **complete_pex_environment.environment_dict(python=python),
    "PEX_EXTRA_SYS_PATH": os.pathsep.join(source_roots),
  • repl:
    chrooted_source_roots = [request.in_chroot(sr) for sr in sources.source_roots]
    extra_env = {
    **complete_pex_env.environment_dict(python=requirements_pex.python),
    "PEX_EXTRA_SYS_PATH": ":".join(chrooted_source_roots),

I don't know if we'll need support in coverage:

@cognifloyd cognifloyd enabled auto-merge (squash) August 11, 2024 00:24
@cognifloyd cognifloyd merged commit 50bba0c into main Aug 11, 2024
@cognifloyd cognifloyd deleted the cognifloyd/pytest-setup-pythonpath branch August 11, 2024 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants