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

Add MacOS support #1045

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add MacOS support #1045

wants to merge 10 commits into from

Conversation

Endle
Copy link

@Endle Endle commented Jan 31, 2024

  1. Load libintl.8.dylib on mac
  2. Check GTK schema

1) Load libintl.8.dylib on mac
2) Check GTK schema
Copy link
Member

@dreua dreua left a comment

Choose a reason for hiding this comment

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

I think this needs a quick test on windows, otherwise looks good to me, thanks for the contribution!

Would you like to add a few lines to the readme as well?

@kbengs
Copy link
Member

kbengs commented Feb 2, 2024

It does not start on Windows because of check_gtk_schema_exists() . Is that needed on Linux and Windows?

@Endle
Copy link
Author

Endle commented Feb 2, 2024 via email

@kbengs
Copy link
Member

kbengs commented Feb 2, 2024

Yes I think so and run the check only on "Darwin", if it is a potential problem there?

Comment on lines 71 to 77
def get_libintl_path():
f = 'libintl.so.8'
if os.name == 'nt':
f = 'libintl-8'
if platform.system() == 'Darwin':
f = 'libintl.8.dylib'
return f
Copy link
Member

Choose a reason for hiding this comment

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

Can we get rid of the variable f here and return directly? Reorder the statements if overwriting was part of the logic but I guess that's not necessary here.
Also add an empty line after the function for readability.

Comment on lines 130 to 137
def check_gtk_schema_exists():
schemas = subprocess.run(["gsettings", "list-recursively"],
stdout=subprocess.PIPE, stderr=subprocess.PIPE,
check=True,
text=True)
return 'org.gtk.Settings.ColorChooser' in schemas.stdout
if not check_gtk_schema_exists():
raise Exception('Found no schema files. You may need to set GSETTINGS_SCHEMA_DIR.')
Copy link
Member

Choose a reason for hiding this comment

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

I totally understand that you added this to help/guide users with the same issue. However we don't want it to break stuff with tests that would be fine without therefore we should either:

  • Run it only on darwin if it is a common enough problem here
  • Make it a warning only. On windows where it is not supposed to work ever we still shouldn't run it because spamming warnings that are unfixable and not helpful are still a bad idea imo.

Feel free to do a combination of both.

@Endle
Copy link
Author

Endle commented Feb 11, 2024

Thank you @dreua

I totally agree with your comments. I'm sorry that I'm a bit distracted in this week. I'll modify my code and add a short note in the README when I have time.

@Endle Endle requested a review from dreua February 17, 2024 01:07
pdfarranger/pdfarranger.py Fixed Show resolved Hide resolved
pdfarranger/pdfarranger.py Dismissed Show dismissed Hide dismissed
Copy link
Member

@dreua dreua left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply: We have another white space at end of line and an undefined Exception type. It would be nice to fix those warnings. Furthermore the tests are failing, we'd need to investigate that as well.

pdfarranger/pdfarranger.py Fixed Show resolved Hide resolved
@@ -118,6 +126,23 @@
from .config import Config
from .core import Sides, _img_to_pdf

def check_gtk_schema_exists():
# On Windows the GTK library has a different logic, so we're skipping the test
# See https://github.com/pdfarranger/pdfarranger/pull/1045/files#r1485570912
Copy link
Member

Choose a reason for hiding this comment

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

Another trialing whitespace, just remove it. Many IDEs or editors have a setting to automatically strip or at least show it.

@dreua
Copy link
Member

dreua commented Feb 26, 2024

Could you rebase the branch on current main? Ideally also getting rid of the merge commits? Jerome recently changed something in the tests maybe that already fixes the failed test here.

@kbengs
Copy link
Member

kbengs commented Feb 29, 2024

I still think this should be darwin only, .ie. if platform.system() == 'Darwin' and not check_gtk_schema_exists():

There is not much reason to run it on Linux. It will slow down upstart of application (by 37ms).

@Endle
Copy link
Author

Endle commented Aug 8, 2024

I'm sorry for my delay. I've updated my PR. May I ask for review it again?

@dreua

@cerenno
Copy link

cerenno commented Aug 14, 2024

Hello. Is it possible to make this into a dmg for mac? I tried all your instructions and can't get it to work. Have also been trying VMs in UTM to try to use it in Linux and can't get it to work. I'm really struggling with this. If there's anything you could do it would be greatly appreciated.

@Endle
Copy link
Author

Endle commented Aug 14, 2024

@cerenno How do you set up dev env on your mac? Nix/homebrew/macPort/etc.. ?

@cerenno
Copy link

cerenno commented Aug 14, 2024

@Endle I use homebrew. I'm not a programmer though. I've had a bit of help but didn't succeed in getting it to work. I think I managed to export the variables and stuff but when it came to using the pip/python there was no module for pdfarranger or similar errors along those lines.

@Endle
Copy link
Author

Endle commented Aug 14, 2024

@cerenno I don't use homebrew but I know it is the most popular option. I'll try to find a method to run pdfarranger with homebrew. Please give me a few days

@cerenno
Copy link

cerenno commented Aug 14, 2024

@Endle Alright. Thank you for taking the time to try. I'll wait for your response.

@Endle
Copy link
Author

Endle commented Aug 14, 2024

@cerenno can you try the following homebrew & pip dependencies?

git clone https://github.com/Endle/pdfarranger.git
cd pdfarranger; git checkout mac
brew install  pygobject3 gtk+3 poppler gettext
ln -s /opt/homebrew/lib/libintl.8.dylib .
pip3 install --user pikepdf PyGObject python-dateutil img2pdf
python3 -m pdfarranger

@cerenno
Copy link

cerenno commented Aug 15, 2024

@Endle Hello. Wow thank you so much! It worked seamlessly. I can't thank you enough for this. One question, for me to open it do I have to type on the terminal cd pdfarranger followed by ./setup.py build python3 -m pdfarranger everytime I want to use it or is there a way to automate it or make it into an icon that you can click and it opens?

@Endle
Copy link
Author

Endle commented Aug 17, 2024

@cerenno Sorry, I don't know packaging in macOS. And I don't know how to send a package request for homebrew (all I can find is https://github.com/orgs/Homebrew/discussions/533)

For now, I think you can set alias for pdfarranger

  1. run echo $0 to tell if your terminal is bash or zsh (ref https://www.moncefbelyamani.com/which-shell-am-i-using-how-can-i-switch/)
  2. If you're using bash, then edit ~/.bash_profile. If you're using zsh, edit ~/.zprofile
  3. Add alias pa='cd xxx; python3 -m pdfarranger' into this file
  4. restart the terminal

Now you should be able to just type pa to start pdfarranger

@Endle Endle requested a review from dreua August 17, 2024 14:06
@cerenno
Copy link

cerenno commented Aug 22, 2024

Hello. Sorry for the delay. It didnt work and adding the alias adjustment makes the app not open, needing to reinstall with the git clone command. When using the command it says that theres no module named pdfarranger and directly after editing the alias, it said something like pdfarranger is an executable or something like that.

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