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

1423 - Better PyApp integration #1547

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

johannesloibl
Copy link

This is a first draft for #1423.

The usage can be seen in the added unit test.

If the implementation is ok, i would continue with documentation updates.

@johannesloibl
Copy link
Author

Alright, implementation should be done. Doing docs now ;)

@johannesloibl
Copy link
Author

Forgot to say, I'm done with my changes.
Maybe you find some time to review.
The changes should be fully backward compatible.

@johannesloibl
Copy link
Author

Gentle reminder that this PR is done from my side and needs review.

@ofek
Copy link
Sponsor Collaborator

ofek commented Jul 17, 2024

this will be in the next release, thank you!

@johannesloibl
Copy link
Author

Awesome, thanks. Looking forward to it :)

@mmorys
Copy link

mmorys commented Aug 27, 2024

Looking through the code and the discussion on #1423, I would like to propose change

Proposed Changes

  • Remove python-version from Options.
  • Ensure that and PYAPP_EXEC_* environment variables are ignored and document that behavior in the docs I noticed that you have already documented this note at the bottom of the binary.md file.

Reasoning

  • I don't think we should have PyApp settings set in both the env-vars section and Options that could lead to ambiguity. There is a PyApp env var called PYAPP_PYTHON_VERSION which makes it possible to define this seemingly identical setting in two different ways, using python-version and PYAPP_PYTHON_VERSION.
  • scripts will presumably override any PYAPP_EXEC_* settings and isn't a direct mapping between an option and environment variable and provides the ability to have multiple binaries, so I think that's ok to leave as an option. It should be documented that this plugin does not use the PYAPP_EXEC_* environment variables.


on_windows = sys.platform == 'win32'
base_env = dict(os.environ)
base_env['PYAPP_PROJECT_NAME'] = self.metadata.name
Copy link

@mmorys mmorys Aug 27, 2024

Choose a reason for hiding this comment

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

I have tested the proposed changes and this does not fix the issue defined in #1436. The project is not actually installed within the PyApp binary, resulting in a No matching distribution error.

According to the PyApp Docs there are 3 ways to define the project path. Here, PYAPP_PROJECT_NAME and PYAPP_PROJECT_VERSION are defined, meaning you are using the Identifier configuration which means "the package will be installed from a package index like PyPI". This is not what we want, as the package is local and not necessarily on PyPi.

Rather than defining these two variables here, the builder should utilize the Embedding config method:

  • Build an sdist or wheel in a temporary directory
  • Set PYAPP_PROJECT_PATH to point to this file
    • Name and version are automatically derived from the sdist/wheel metadata.

See my comment on the related issue for how I got around this issue by building and pointing to an sdist.

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.

None yet

3 participants