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

Remove windows-only logic from the action #95

Merged
merged 1 commit into from
Oct 12, 2022
Merged

Remove windows-only logic from the action #95

merged 1 commit into from
Oct 12, 2022

Conversation

sondrelg
Copy link
Member

@sondrelg sondrelg commented Oct 7, 2022

Removes windows-only logic from the action. This seems not to be needed.

@sondrelg
Copy link
Member Author

sondrelg commented Oct 7, 2022

Yeah intending to reproduce, then fix, then merge. Just preemptively set the Resolves #94 to it's there when we do get to the bottom of this 👍

main.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@miigotu miigotu left a comment

Choose a reason for hiding this comment

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

3.11 is going to be released this month, we should probably test the latest rc2

@miigotu
Copy link
Collaborator

miigotu commented Oct 11, 2022

On windows, I thought the path is $path/Scripts/poetry.exe
And Linux/Mac:
$path/bin/poetry

As this sits, on windows it tries $path/Scripts/bin/poetry.exe, is that correct?

@miigotu
Copy link
Collaborator

miigotu commented Oct 11, 2022

Could we add a temporary [[ -f "$poetry_" ]] || echo "Poetry not found" && exit 1
Here? Just for my piece of mind 😂
https://github.com/snok/install-poetry/compare/470a7d95f5d20f3d61351fc140809338bee18bdf..f9fc317f01f108f1b25d09cfbe8b58efa2b874d9#diff-f121df8f1f148ef72415f98dd27089a0e6cec3ec939840286358170e5e20c990R49
(Right before calling the executable to set options)

@miigotu
Copy link
Collaborator

miigotu commented Oct 11, 2022

Scripts dir is symlinked to bin dir now:
Screenshot_20221010-220443

This is working now, some of the changes probably can be undone, but we don't need a special path for windows anymore:
https://github.com/miigotu/install-poetry/pull/1/files

@sondrelg
Copy link
Member Author

Yep I can do that 👍

Co-authored-by: miigotu <miigotu@gmail.com>
@sondrelg sondrelg mentioned this pull request Oct 11, 2022
@sondrelg sondrelg marked this pull request as ready for review October 11, 2022 08:13
@sondrelg
Copy link
Member Author

Happy with the changes here @miigotu? Moved all other changes, and a few extra, to #97

@sondrelg sondrelg changed the title Fix windows 3.8 issue Remove windows-only logic from the action Oct 11, 2022
@sondrelg
Copy link
Member Author

Looks like these changes do not fix the 3.8 issues. The general test suite did not cover the issue. Think we should merge this and #97 as they are and continue that in #98

@miigotu miigotu mentioned this pull request Oct 11, 2022
@sondrelg sondrelg merged commit a323b40 into main Oct 12, 2022
@sondrelg sondrelg deleted the 94 branch October 12, 2022 01:18
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.

3 participants