-
Notifications
You must be signed in to change notification settings - Fork 502
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
Update readme and fix python tests #238
base: master
Are you sure you want to change the base?
Update readme and fix python tests #238
Conversation
``` test/conftest.py:66 sshfs/build/test/conftest.py:66: PytestDeprecationWarning: @pytest.yield_fixture is deprecated. Use @pytest.fixture instead; they are the same. @pytest.yield_fixture(autouse=True) ```
Warning: ``` test/util.py:99 sshfs/build/test/util.py:99: PytestUnknownMarkWarning: Unknown pytest.mark.uses_fuse - is this a typo? You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/mark.html return pytest.mark.uses_fuse() ``` References for the fix: 1. https://stackoverflow.com/questions/60806473/pytestunknownmarkwarning-unknown-pytest-mark-xxx-is-this-a-typo/60813297#60813297 1. https://docs.pytest.org/en/stable/mark.html
Changes to be committed: modified: ../README.md new file: ../test/README.md
To see what the new readme looks like, look at my repo here: https://github.com/ElectricRCAircraftGuy/sshfs/tree/update_readme. |
Why is Travis CI failing? It might be because it is set to use an old Python 3 version (3.5)? instead of something newer? |
### 1. General overview | ||
|
||
First, download the latest SSHFS release from https://github.com/libfuse/sshfs/releases. On Linux | ||
and BSD (as opposed to MacOS), you will also need to install [libfuse][libfuse] 3.1.0 or newer. On |
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.
On MacOS, you need to install OSXFUSE I believe.
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.
This is stated just one line below.
README.md
Outdated
MacOS, you need [OSXFUSE][OSXFUSE] instead. Finally, you need the [Glib][Glib] library with | ||
development headers, which should be available from your operating system's package manager. | ||
|
||
To build and install, we recommend you use [Meson][Meson] (version 0.38 or newer) and |
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 think that's a requirement, not a recommendation.
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.
fixed
_Tested 22 Dec. 2020 on Ubuntu 20.04._ | ||
|
||
1. Download and `cd` into the source code | ||
1. To download the latest source code: |
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.
The people who install from Git should not need these instructions, so I think this can be safely dropped.
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'm trying to write with the lowest common denominator in mind, meaning even for the beginner developer who may need this help. With your permission, I'd like to leave these details. Just a handful of years ago I needed these instructions myself, and not having many repos with these instructions slowed my progress into making contributions.
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.
That makes sense, thanks for explaining. How about keeping the instructions but moving them into a separate file ("contributor_info.md" or something like that?). That way, the information is available for those who need it without getting in the way of people who don't need it.
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.
👍, moving how to build from git to CONTRIBUTING.md
would be good.
# run the python3 tests in the "test" dir | ||
python3 -m pytest test/ | ||
``` | ||
For [sample test output, see here](test/README.md). |
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.
Not sure why it's important to have example output? The tests clearly state if they fail or pass....
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.
It took my a lot of effort and time to get the tests to pass, as you can see from my ssh setup instructions, so I was never able to see what was being tested or what was supposed to happen until I had solved many problems and gotten the tests to run. This removes that mystery.
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.
That being said, there's not a ton of value in it. It's just nice to see what's supposed to happen is all.
README.md
Outdated
```bash | ||
sudo ninja install | ||
``` | ||
1. "Light" method. This technique simply creates a symlink to the executable in your `~/bin` |
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.
Please drop this. People who want to do something like this should know how to do it without needing instructions.
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.
Is this change blocking? If so, I'll remove it. If not, this info. teaches people who want to do this but don't know how. It took me years to figure this out.
sudo ninja install | ||
``` | ||
1. "Light" method. This technique simply creates a symlink to the executable in your `~/bin` | ||
dir so your Linux distro's install still remain's intact and untouched. (For Ubuntu). |
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.
This is the case also when using ninja install. Your system's sshfs is in /usr/bin, ninja install into /usr/local/bin.
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.
What's the ninja uninstall command?
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 don't know. But I also don't see the connection to what I'm saying...? If you install in /usr/locl, then your Linux distro's install remains intact and untouched.
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.
If you install in /usr/locl, then your Linux distro's install remains intact and untouched.
Agreed. So, if someone wants to go back to using their Linux distro's version of sshfs
, how do they do that? The logical answer is: run the ninja uninstall command. That's why I asked "what's the ninja uninstall command?" I'd like to write it down here too.
For the "Alternative method" here, I show: rm ~/bin/sshfs
.
A few options to uninstall the ninja install
might be:
rm /usr/local/bin/sshfs
Or: modify the path to include /urs/local/bin
after /usr/bin
instead of before it (probably not recommended), or (best, if such a thing exists): sudo ninja uninstall
.
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.
Have you tried running ninja uninstall
? If I understand https://mesonbuild.com/Release-notes-for-0-38-0.html correctly, then this should exist and is probably the best approximation to a true uninstall that we have.
Looks like the pytest version that is being installed isn't compatible with Python 3.5. We'll have to either pin to an older pytest version, or test with a newer Python release. |
Python 3.5 is really old. Let's test with a newer Python release. How do we do that? |
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.
@Nikratio , I addressed comments. Let me know if these unchanged items are blocking.
README.md
Outdated
MacOS, you need [OSXFUSE][OSXFUSE] instead. Finally, you need the [Glib][Glib] library with | ||
development headers, which should be available from your operating system's package manager. | ||
|
||
To build and install, we recommend you use [Meson][Meson] (version 0.38 or newer) and |
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.
fixed
# run the python3 tests in the "test" dir | ||
python3 -m pytest test/ | ||
``` | ||
For [sample test output, see here](test/README.md). |
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.
That being said, there's not a ton of value in it. It's just nice to see what's supposed to happen is all.
@Nikratio , which items are still blocking? I'll address any blocking issues to ensure you are okay merging it. I can add your proposed I'd like to have Travis CI use a later Python version. I'm not entirely sure how to do that. Some ideas include:
I prefer option 1 if that works as I think it will. Is there any reason not to update to 18.04, So, what do I need to do to get this PR merged? Which items are blocking vs recommended but not blocking? |
I think updating the CI to bionic is fine. If you can move the alternative installation options to a separate file, I'd be happy to merge this. |
Any update on this? |
@Nikratio , I haven't had time to look at this again yet. Still on my todo list. If you'd like to make the fixes and merge, feel free to follow the github "command-line" instructions you should be able to see here by the merge button, then I believe you can check out my branch, make any changes yourself you see fit, and push to the master branch. If you are unable to take it from here and make the fixes, I'll get back to it when able. Note that so long as you keep my commits intact and just add new commits on top of my commits to fix my commits, I will still show up as a contributor, so you making corrections to my PR here won't take that away. |
Are you still interested in working on this? If not, I'll close the pull request in a few days. |
@Nikratio , Would you be able to manually check out my branch, add the changes you'd like, and manually merge them to the main branch? If so, you get the fixes you want and I get credit for the parts of the PR you kept. |
This is a major improvement to the readme, significantly improving build instructions. I also improve general usage instructions, and I port the readme to Github Flavored Markdown, which is more widely-used, widely-understood, and widely-supported. Lastly, I fix two python test warnings, See commit a1d58ae and 03ee1f8 for detailed descriptions.