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

Implement find_libreoffice() #92

Closed
nanxstats opened this issue Mar 9, 2022 · 7 comments
Closed

Implement find_libreoffice() #92

nanxstats opened this issue Mar 9, 2022 · 7 comments
Assignees

Comments

@nanxstats
Copy link
Collaborator

This is a further fix for #64.

The overarching goal is to add flexibility to set up the path via a function argument or environment variable instead of fully automatic discovery. Sometimes, this would require adding a one-liner user-land code to point to the correct path (say if libreoffice is installed into nonconventional locations) but would make the function generalized enough and exportable later.

There should be a new function called find_libreoffice(), called by r2r2f:::rtf_convert_format(). Here is a design proposal for find_libreoffice():

  1. The general logic and style for discovery should be similar to rmarkdown::find_pandoc() and rmarkdown::find_program().

  2. Fallback order

    • A dir or path argument allowing the path to be set at the function call level
    • An environment variable named R2RTF_LIBREOFFICE that you can set globally outside of the function; the environment variable PATH (need to check if this is actually used by standard libreoffice installations)
    • Use the highest version found if it's not set using the function argument
    • Check if the version is >= 7.1
  3. Other details

    • Remove the specific version numbers in the current implementation such as 7.2, 7.1.
    • Give informative feedback on how to install libreoffice under Ubuntu (apt), Windows (choco), and Mac (brew) if not found.
  4. Testing environments

    • Linux
    • GitHub Actions
    • Windows
    • Mac

@elong0527 you can assign this to me.

@elong0527
Copy link
Collaborator

I will take this generous offer :)

The information below from CRAN maintainer might be helpful.

Fedora 34 does have LibreOffice 7.1.8, and the check machine has it
installed.

I fixed the issue in 0011490 by skipping a test

@cole-johanson
Copy link

FYI - LibreOffice 7.3 was released in August 2022, so this line is currently preventing the package from working.

@fb-elong
Copy link
Collaborator

@nanxstats, how about we add sys_cmd as a function argument to allow user specify the command name?

@nanxstats
Copy link
Collaborator Author

@fb-elong I like the creativity. That seems an easy way out, but I'm not convinced it will generalize well.

My reasoning: this is essentially getting/setting a global state. Making it localized (required or optional) might bring more issues than it solves.

Let's use compiling this book as an example. If we set a hard-coded sys_cmd path in function calls for GitHub Actions, users have to change the code to make it compile in local environments. In contrast, having a detection/fallback logic where users are allowed to manually set libreoffice path via options() would be more canonical, without changing the content.

@fb-elong
Copy link
Collaborator

Got it.

In preparing a newer version, would you be able to find a robust way to enable LibreOffice 7.3 or beyond?

@nanxstats
Copy link
Collaborator Author

@fb-elong I'm quite sure there is no major technical challenges.

I cannot promise a timeline though - it's not a trivial amount of work, not to mention the time spent on testing on ALL mainstream platforms. So we should not let this become a blocker for any releases plans.

elong0527 added a commit that referenced this issue Feb 1, 2023
@elong0527 elong0527 mentioned this issue Feb 1, 2023
@nanxstats
Copy link
Collaborator Author

Closing this issue as 7.3 support has been added. We have also agreed that we may not need a high-effort solution for this internal function which does not provide the core functionality of the package.

@nanxstats nanxstats closed this as not planned Won't fix, can't repro, duplicate, stale Feb 1, 2023
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

No branches or pull requests

4 participants