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

Fix locating jabref in jabrefHost.py #6963

Merged
merged 1 commit into from
Oct 3, 2020

Conversation

michaellass
Copy link
Contributor

shutil.which() returns an str and not a pathlib.Path. Therefore, we need to check if which() actually returns something and if so, we should convert it to a pathlib.Path such that JABREF_PATH is always of the same type.

I stumbled across this while fixing JabRef/JabRef-Browser-Extension#158 for the unofficial Arch Linux JabRef package in the Arch User Repository (AUR): michaellass/AUR#17

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@michaellass
Copy link
Contributor Author

Would it make sense to test for shutil.which("JabRef") as well here? To me it seems somehow inconsistent to expect either /opt/jabref/bin/JabRef or lowercase jabref in $PATH.

@Siedlerchr
Copy link
Member

Thanks for fixing this!
@LyzardKing can you take a look please?

@LyzardKing
Copy link
Collaborator

Looks good! Thanks for pointing out the pathlib issue.
I think we can add the uppercase version as well, just to be on the safe side.

@michaellass michaellass force-pushed the fix-jabrefHost.py branch 2 times, most recently from cdc85b9 to fcd084d Compare October 1, 2020 18:58
@LyzardKing
Copy link
Collaborator

I wonder if setting an if-else chain is easier to read...
Especially if we need to add different paths (I'm thinking of the flatpak now, since I still have to properly figure out that integration...)

Something like:

relpath_path = script_dir / "bin/JabRef"
lowercase_path = shutil.which("jabref")
uppercase_path = shutil.which("JabRef")
if relpath_path.exists():
    JABREF_PATH = relpath_path
elif lowercase_path is not None and os.path.exists(lowercase_path):
    JABREF_PATH = Path(lowercase_path)
elif uppercase_path is not None and os.path.exists(uppercase_path):
    JABREF_PATH = Path(uppercase_path)
else:
    logging.error("Could not determine JABREF_PATH")
    sys.exit(-1)

shutil.which() returns an str and not a pathlib.Path. Therefore, we need
to check if which() actually returns something and if so, we should
convert it to a pathlib.Path such that JABREF_PATH is always of the same
type.

Also, try locating JabRef in addition to jabref in the user's PATH.
@michaellass
Copy link
Contributor Author

I think that's a good idea to avoid the deep nesting. I updated the PR using your suggested code.

@@ -44,6 +44,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue where no longer a warning was displayed when inserting references into LibreOffice with an invalid "ReferenceParagraphFormat". [#6907](https://github.com/JabRef/jabref/pull/60907).
- We fixed an issue where a selected field was not removed after the first click in the custom entry types dialog [#6934](https://github.com/JabRef/jabref/issues/6934)
- We fixed an issue where a remove icon was shown for standard entry types in the custom entry types dialog [6906](https://github.com/JabRef/jabref/issues/6906)
- We fixed an issue with the python script used by browser plugins that failed to locate JabRef if not installed in its default location.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- We fixed an issue with the python script used by browser plugins that failed to locate JabRef if not installed in its default location.
- We fixed an issue with the python script used by browser plugins that failed to locate JabRef if not installed in its default location. [#6963](https://github.com/JabRef/jabref/pull/6963/files)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@Siedlerchr
Copy link
Member

Please let us know if this is ready for merge.

@koppor koppor merged commit a33466f into JabRef:master Oct 3, 2020
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.

4 participants