-
-
Notifications
You must be signed in to change notification settings - Fork 96
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
Python scripts: compatibility with Windows #446
Python scripts: compatibility with Windows #446
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, João. Please see my comments.
Hi @maxim-belkin, I got a bit lost with all the comments and diverging conversations on the PR so I'm summarizing here. This PR addresses compatibility with Windows systems (at least for Win 10, which I can test) by fixing 3 issues:
I believe this PR, as is, addresses all three points coherently. The change in the As for the other changes that might be considered cosmetic, to be completely honest, I had an incredibly hard time understanding (at a glance) what some methods were doing in this code. If I changed things, it was because I had to rewrite parts of the code to understand how to fix it. I believe the result is more readable and more maintainable. As such, I am not willing to work on this PR further. You're welcome to fork my repo and cherry-pick the changes and commit them yourself to the main branch/repo. I spent a more than a few hours figuring out what the problem was and how to fix it, then working on a PR, and writing an issue that rivals chapters of my PhD thesis in length. The lessons are up and running on my laptop, that's all I wanted. Fixing it for everybody else was a bonus. You're all volunteers with limited time to devote to this project, but so am I. I hope you understand my point of view. |
Thanks for your contribution, @JoaoRodrigues. I'll take the PR from here, so please don't delete your fork and the branch you used to submit the PR from (win10_compatibility). Regarding your last comment: conversations are not diverging, but there are quite a few of them, which makes things hard to follow. The usual recommendation (and a common practice) is to submit smaller pull requests. They are easier to discuss and usually take less time and effort on both sides (contributors' and maintainers'). |
These will be submitted in a separate PR
We can't use a single shebang: * on some platforms `python` may mean Python 2, on others - Python 3 * on some platforms `python3` does not exist at all Therefore, we're removing the shebangs altogether.
f5b80b7
to
1a0adfd
Compare
... for compatibility with Windows
I think I caught all remaining Windows incompatibilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I improved one comment to make it more complete, but otherwise this looks good to me; noting that I cannot test on windows
By this time I had already run "python bin/lesson_initialize.py"
When I viewed my repo with
Then, being new to this, I tried to re-apply the patch, and there were many errors including:
I'm going to delete the local repo, and re-clone it from my GithubMy machine: I'm using a Win10 machine and GitBash. When I type:
I have Anaconda3 (64-bit) installed, although when I open the Anaconda (or GitBash) terminal and type:
Others:
Through Anaconda Navigator I have: |
Warnings are normal in this case because the
This is strange: it shouldn't list
If you look at the
And gem install json kramdown After that you should be able to execute lesson checks manually like this (GitBash): # cd to the root of your lesson repository
python bin/lesson_check.py -s . -p bin/markdown_ast.rb -r _includes/links.md |
|
the method it says is missing is new, via (https://stackoverflow.com/questions/55551191/module-yaml-has-no-attribute-fullloader), that issue shows how to check and update it if you use pip. There is a way to do it with conda too, if you prefer since @hoytpr also has a python3 alias, maybe his install of anaconda was older than Joao's and anaconda changed how it sets the alias? |
As Sarah explained, this error messages is caused by the outdated PyYAML module. conda update You might also want to clean up unused packages after this: conda clean --all
I think you're right. conda update conda
conda update anaconda You might need to restart GitBash after that. See if you still have |
Chiming in to add info on this issue of the python call. This answer on SO seems to explain why there is not python3.exe on Windows and provides an alternative: https://stackoverflow.com/a/34223417 In addition, would you be interested in setting up a CI like appveyor or Travis to check if the make commands pass on Linux, Win, and MacOS? I would be happy to work on that to improve testing/debugging these issues. |
FYI: The alias appears to have been installed as part of a Windows update for "Zune" (including iHeartRadio) on Nov 5, 2019.
Because this looked to only update PyYaml to 4.7, I ran
Went ahead with resolving the inconsistencies, and the next thing I know I have 299
Went ahead with this and then:
|
Little advice please.
Should I continue? |
These are outdated packages, so it should be safe to remove them. |
Okay, this is where I am now;
|
This is a successful test. The error message means that issues: https://some.location |
Looks good. Adding
to the bottom of
-p |
This is good news. Basically, you were able to detect and fix an issue in |
I still get an error about python3:
|
NOTE that I have "python3" environment variable set as EDIT: No, restarting the machine still gives this error.
|
Thanks, Pete. Yes, that |
I turned off the aliases using the SO site suggestion, and re-ran
|
|
So disabling the Windows aliases does work, although when the makefile asks |
I've added the logic that should deal with Python3 from the MS Store. Could you please test it, Pete? You should be able to "restore" your repository to the state before applying the patch with: git checkout -- .
rm 446.patch After that, you should be able to re-download the patch with |
EDIT: I re-cloned my GitHub repo to be sure everything was clean: Got the patch:
With a caveat it seemed to work:
without prepending the command with
So I then ran:
And got no response at all. I assume this is good.
So I then ran:
Looks good! |
Rendered fine.
|
This is expected: when you applied the patch, Git informed you that it expected Python scripts to not have executable permission bits but found that your scripts still have them. Git applied the patch anyway but did not change their executable permission bits -- that's why you still can execute the scripts (e.g., chmod 0644 bin/*.py # or: chmod a-x bin/*.py You will no longer be able to execute the scripts and Git Bash will tell you something like this: -bash: ./bin/lesson_check.py: Permission denied
Yes, this is good. Notifying a user of a successful initialization is a reasonable thing to do but this is out of scope of this PR.
Wonderful! |
This PR includes several commits that improve compatibility of the code on windows machines. For a detailed description of the problems and fixes, see #445 .