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

Handle NotImplementedError when creating symlink on install; fixes #4626 #4630

Closed
wants to merge 3 commits into from

Conversation

havocbane
Copy link

Handle NotImplementedError when creating symlink on install. I was able to recreate the issue locally with PyPy version 3.7.10 and Poetry version 1.1.11 and I verified my fix after making changes: python install-poetry.py completed successfully with this change in place. If accepted, please tag the merge with the label hacktoberfest-accepted.

Resolves: #4626

  • Added tests for changed code.
  • Updated documentation for changed code.

I am happy to add a test over this change if requested, but I don't see where tests covering the install-poetry.py script live in the repository.

@mattip
Copy link

mattip commented Oct 13, 2021

I don't know if the poetry CI is already taking too much time, but you could add runs for python3.10 and pypy3.7 by changing

python-version: [3.6, 3.7, 3.8, 3.9]

to (note the quotes to force a string, not a number, critical for 3.10)

python-version: ["3.6", "3.7", "3.8", "3.9", "3.10", "'pypy-3.7']

@havocbane
Copy link
Author

I don't know if the poetry CI is already taking too much time, but you could add runs for python3.10 and pypy3.7 by changing

python-version: [3.6, 3.7, 3.8, 3.9]

to (note the quotes to force a string, not a number, critical for 3.10)

python-version: ["3.6", "3.7", "3.8", "3.9", "3.10", "'pypy-3.7']

Thanks for the suggestion. The CI is just waiting for a maintainer to kick it off since I have not contributed to this codebase before. Also, since this change is focused on pypy, I will omit 3.10 from the update.

.github/workflows/main.yml Outdated Show resolved Hide resolved
@abn
Copy link
Member

abn commented Oct 28, 2021

The failed test is actually what the PR is fixing. The bootstrap uses the code from the project's default branch, this should pass once merged.

https://github.com/python-poetry/poetry/runs/4038721220?check_suite_focus=true#step:5:34

Copy link
Member

@abn abn left a comment

Choose a reason for hiding this comment

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

Looks like the CI runs are failing for OSX + pypy. Will need to fix those before we can merge this.

Ref: https://github.com/python-poetry/poetry/runs/4038720998?check_suite_focus=true

Might also be good to split out the install script changes and the github action changes into separate commits. Something like:

installer: handle NotImplementedError on symlink

Resolves: #4626
ci: add pypy to test matrix

.github/workflows/main.yml Outdated Show resolved Hide resolved
@havocbane
Copy link
Author

I'm not sure why the FreeBSD checks are failing installing tox. Could it be due to the pinning of the PyPi version? It looks unrelated to me.

@abn
Copy link
Member

abn commented Oct 30, 2021

@havocbane try rebasing the branch, #4668 should remove tox usage.

@havocbane
Copy link
Author

@havocbane try rebasing the branch, #4668 should remove tox usage.

Thanks! The FreeBSD checks are passing now.

@abn
Copy link
Member

abn commented Nov 12, 2021

@havocbane can you resolve the install-poetry conflicts please?

@neersighted neersighted self-assigned this Nov 12, 2021
@@ -322,8 +322,11 @@ def _get_release_info(self, name: str, version: str) -> dict:
return data.asdict()

def _get(self, endpoint: str) -> Union[dict, None]:
headers = {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like some unrelated changes came along for the ride. Could we back these out, and open a separate PR with tests if they are necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, good catch. I was trying to debug the mac failures from the CI. I'll back these out in a new pull request.

@neersighted
Copy link
Member

Given the extra changes that came along, and the fact that the installer script has been factored out, I am going to close this in favor of a PR at https://github.com/python-poetry/install.python-poetry.org.

For anyone using the old URL, they will not get the improved version of this script after merge. However, I would prefer an issue on that repo to discuss if we'll keep the version in the main poetry repository in sync, or something else.

@hoefling
Copy link
Contributor

I encountered the same issue with being unable to install poetry on win + pypy3. @havocbane was there a reason you didn't port this PR to https://github.com/python-poetry/install.python-poetry.org?

@havocbane
Copy link
Author

I encountered the same issue with being unable to install poetry on win + pypy3. @havocbane was there a reason you didn't port this PR to https://github.com/python-poetry/install.python-poetry.org?

@hoefling Unfortunately, life kind of got in the way and I didn't keep up with the changes here. If this is still an issue, I can make a change over there. I know a lot of time has passed since this came up, so sorry for that.

@mattip
Copy link

mattip commented Nov 4, 2023

I encountered the same issue with being unable to install poetry on win + pypy3

What version of pypy3? The latest pypy7.3.13 added support for symlinks in windows, so it should work now.

Copy link

github-actions bot commented Mar 3, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

symlink can raise NotImplementedError
5 participants