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

gftools unable to handle folder names with spaces? #252

Closed
tiroj opened this issue Oct 14, 2020 · 14 comments
Closed

gftools unable to handle folder names with spaces? #252

tiroj opened this issue Oct 14, 2020 · 14 comments

Comments

@tiroj
Copy link

tiroj commented Oct 14, 2020

I'm getting an error message when running the build script for the Castoro fonts, and I suspect this may be due to my local folder structure having a word space in one of the subfolders in the directory path:

= Post procesing TTF
build.sh: /Users/tiro_j/Documents/Work Files/Git repos/Castoro/venv/bin/gftools: /Users/tiro_j/Documents/Work: bad interpreter: Permission denied

Can someone confirm whether this is in fact the probably cause, and whether gftools could be updated to handle folder names containing spaces? I doubt if I am alone in having some folders with spaces in the name.

@chrissimpkins
Copy link
Member

chrissimpkins commented Oct 15, 2020

I reproduced this with macOS 10.15.7 / cPython 3.8.6 in a clone of your repo that includes a venv directory path with spaces. It definitely looks like the venv installed executables do not like spaces in directory paths. You might check the fontmake that you are using within the venv too. In my activated venv it looks like this is the system installed version of fontmake and not the venv installed version. That could be an important issue if you intend to fix the compiler version in your build workflow.

A workaround for absolute paths with spaces is to use explicit relative file paths to the venv installed executable sources. This works for me and should support anyone who builds from a virtual environment directory that is named venv at the root of your repository:

src/build.sh

#!/bin/sh
set -e

mkdir -p ../fonts/otf ../fonts/ttf

echo = "Generating TTFs"
../venv/bin/fontmake -u ./UFO/Castoro-Regular.ufo -o ttf --output-dir ../fonts/ttf -a
../venv/bin/fontmake -u ./UFO/Castoro-Italic.ufo -o ttf --output-dir ../fonts/ttf -a
# fontmake -m ./UFO/Castoro_Roman.designspace -o ttf --output-dir ../fonts/ttf -a
# fontmake -m ./UFO/Castoro_Italic.designspace -o ttf --output-dir ../fonts/ttf -a

echo = "Post procesing TTF"
ttfs=$(ls ../fonts/ttf/*.ttf)
for ttf in $ttfs
do
    python ../venv/bin/gftools-fix-hinting.py $ttf
    mv "$ttf.fix" $ttf
    python ../venv/bin/gftools-fix-dsig.py -f $ttf;
    python3 castoro_stat_table.py $ttfs
done

echo = "Generating OTFs"
../venv/bin/fontmake -u ./UFO/Castoro-Regular.ufo -o otf --output-dir ../fonts/otf
../venv/bin/fontmake -u ./UFO/Castoro-Italic.ufo -o otf --output-dir ../fonts/otf
# fontmake -m ./UFO/Castoro_Roman.designspace -o otf --output-dir ../fonts/otf
# fontmake -m ./UFO/Castoro_Italic.designspace -o otf --output-dir ../fonts/otf

echo "Post processing static OTFs"
otf=$(ls ../fonts/otf/*.otf)
for otf in $otf
do
	python ../venv/bin/gftools-fix-dsig.py -f $otf;
    ../venv/bin/psautohint $otf;
done

echo "Done!"

@vv-monsalve
Copy link

Thanks, Chris, for the detailed answer.

in a clone of your repo that includes a venv directory path with spaces

Do you mean a venv included in the repository? I'm curious since there was one, but I deleted it as it is not the suggested practise to include it in the repository.

It definitely looks like the venv installed executables do not like spaces in directory paths

So, for clarification, the main source of conflict in this case would be the spaces in directory paths. It wouldn't matter if a new venv is created for the directory it would still have those, that's why in this case including the path to ../venv would be the best solution. Is that it?

A workaround for absolute paths with spaces is to use explicit relative file paths to the venv installed executable sources.

Thanks, I'll try that and let you know the outcome :)

@graphicore
Copy link
Contributor

I can totally reproduce this. Seems to me venv related.

@vv-monsalve
Copy link

I can totally reproduce this. Seems to me venv related.

Which venv did you use? A locally generated one?
I changed the build following Chris workaround. Under my local venv both, the previous and the current build worked for me locally,

@chrissimpkins
Copy link
Member

chrissimpkins commented Oct 16, 2020

Thanks, Chris, for the detailed answer.

in a clone of your repo that includes a venv directory path with spaces

Do you mean a venv included in the repository? I'm curious since there was one, but I deleted it as it is not the suggested practise to include it in the repository.

You can add venv to your .gitignore. It should not be pushed to the remote. But it is ok to include it in repo locally IMO. Not aware of guidance about the practice out there.

It definitely looks like the venv installed executables do not like spaces in directory paths

So, for clarification, the main source of conflict in this case would be the spaces in directory paths. It wouldn't matter if a new venv is created for the directory it would still have those, that's why in this case including the path to ../venv would be the best solution. Is that it?

It appears to be the spaces in the absolute directory path that John was using above the level of the repository directory. I tested it by adding a space in the repository directory name and, in a separate test, the venv directory name. It caused issues in both cases.

A workaround for absolute paths with spaces is to use explicit relative file paths to the venv installed executable sources.

Thanks, I'll try that and let you know the outcome :)

G/L!

@chrissimpkins
Copy link
Member

chrissimpkins commented Oct 16, 2020

I can totally reproduce this. Seems to me venv related.

Which venv did you use? A locally generated one?
I changed the build following Chris workaround. Under my local venv both, the previous and the current build worked for me locally,

Users should always be creating their virtual environment locally. You don't want to commit your venv directory to your git version control.

@vv-monsalve
Copy link

You can add venv to your .gitignore. It should not be pushed to the remote. But it is ok to include it in repo locally IMO. Not aware of guidance about the practice out there.

Users should always be creating their virtual environment locally. You don't want to commit your venv directory to your git version control.

Agreed, it's what was suggested and done, to create it locally. Also why the venv was removed from the upstream repository and created a .gitignore with it included.

It appears to be the spaces in the absolute directory path that John was using above the level of the repository directory. I tested it by adding a space in the repository directory name and, in a separate test, the venv directory name. It caused issues in both cases.

Thanks! This must be the reason then.

@graphicore
Copy link
Contributor

Seems related: pypa/virtualenv#53

@tiroj
Copy link
Author

tiroj commented Oct 16, 2020

I don’t know if related, but I am also getting a permission error. This is running the latest merge of the build script from @vv-monsalve on Mac OS.

= Post procesing TTF
Traceback (most recent call last):
  File "../venv/bin/gftools", line 99, in <module>
    p = subprocess.Popen(args, stdout=sys.stdout,
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/subprocess.py", line 854, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/subprocess.py", line 1702, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
PermissionError: [Errno 13] Permission denied: '../venv/bin/gftools-fix-hinting.py' 

@chrissimpkins
Copy link
Member

Seems related: pypa/virtualenv#53

😆 From that thread. Life in 2020...

2020-10-16_13-39-17

@chrissimpkins
Copy link
Member

chrissimpkins commented Oct 16, 2020

I don’t know if related, but I am also getting a permission error. This is running the latest merge of the build script from @vv-monsalve on Mac OS.

= Post procesing TTF
Traceback (most recent call last):
  File "../venv/bin/gftools", line 99, in <module>
    p = subprocess.Popen(args, stdout=sys.stdout,
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/subprocess.py", line 854, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/subprocess.py", line 1702, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
PermissionError: [Errno 13] Permission denied: '../venv/bin/gftools-fix-hinting.py' 

Ugh... @vv-monsalve @tiroj I suggest trying a change of all of the python [path]/gftools [sub-command] calls to python [path]/[gftools script that is called by gftools executable]. These file paths are formed by replacing the space between gftools and [SUB-COMMAND] with a hyphen and adding a .py. e.g.,

python ../venv/bin/gftools fix-hinting [FONT]

becomes

python ../venv/bin/gftools-fix-hinting.py [FONT]

You'll find the full list of script names that correspond with gftools sub-command names in https://github.com/googlefonts/gftools/tree/master/bin

@vv-monsalve
Copy link

vv-monsalve commented Oct 16, 2020

Oh! I missed that! Sorry, my bad. I fixed it here TiroTypeworks/Castoro#22
I tried the previous build before pushing it and was "ok". I guess because I did it under my local folder structure that doesn't have spaces.

@vv-monsalve
Copy link

As for the original question of the Issue

gftools unable to handle folder names with spaces?

After we all reproduced it, it seems that the source of the problem was this indeed:

this may be due to my local folder structure having a word space in one of the subfolders in the directory path.

In these cases the workaround suggested by Chris: "to use explicit relative file paths to the venv installed executable sources" would be needed for the builds, it worked at the end!
Thanks a lot, @chrissimpkins and @graphicore for your contributions!

@tiroj
Copy link
Author

tiroj commented Oct 18, 2020

Yes, thank you very much everyone.

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

5 participants