Skip to content

Conversation

@zanieb
Copy link

@zanieb zanieb commented Nov 11, 2023

The provided suggestion to run ./install_macos.sh from the util
directory fails since the script expects to be relative to the
root of the content.

❯ cd util
❯ bash ./install_macos.sh
cp: ./fonts/otf/*: No such file or directory
cp: ./fonts/variable/*: No such file or directory

Avoiding the change to util and running bash ./util/install_macos.sh
works.

A similar pull request exists for the Linux installation instructions at #104

The provided suggestion to run `./install_macos.sh` from the `util`
directory fails since the script expects to be relative to the
root of the content.

```
❯ cd util
❯ bash ./install_macos.sh
cp: ./fonts/otf/*: No such file or directory
cp: ./fonts/variable/*: No such file or directory
```

Avoiding the change to `util` and running `bash ./util/install_macos.sh`
works.
@shivan-s shivan-s mentioned this pull request Nov 12, 2023
@shivan-s
Copy link

Excellent PR.

This was referenced Nov 16, 2023
@brunofin
Copy link
Contributor

brunofin commented Nov 17, 2023

not sure why this was changed, I opened #26 (PR #29) for Linux installation and by the time my PR was merged, both MacOS and Linux instructions used bash ./util/install_XXX.sh instead. This PR needs to be merged, and if you could please also fix the Linux instructions the same way I'd be grateful.

@brunofin
Copy link
Contributor

those changes were introduced by @idan here 45c9887 upon merging my PR.

@zanieb
Copy link
Author

zanieb commented Nov 17, 2023

Someone fixed the Linux instructions in #104 — there are already a bunch of duplicates so I'll just leave it separate.

@sylver sylver mentioned this pull request Nov 19, 2023
```bash
$ cd util
$ bash ./install_macos.sh
$ bash ./util/install_macos.sh
Copy link

Choose a reason for hiding this comment

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

Suggested change
$ bash ./util/install_macos.sh
$ bash util/install_macos.sh

Copy link
Author

@zanieb zanieb Nov 28, 2023

Choose a reason for hiding this comment

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

Can you explain your suggestion? Being explicit about using the current directory is clearer and consistent with the existing style i.e. with this change it would not match the Linux installation instructions.

Copy link

Choose a reason for hiding this comment

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

Being explicit about using the current directory is clearer

I personally find NOT specifying the current directory clearer ¯\_(ツ)_/¯ there's a tiny bit less stuff to parse by our brains

consistent with the existing style i.e. with this change it would not match the Linux installation instructions

didn't notice 😄 in that case I'd personally also remove it from Linux installation instructions; I simply find it unnecessary as bash install_linux.sh already tells me the file is in the cwd :)

I'm fine with it being bash ./util/install_macos.sh though, it's really just a nit

@zanieb zanieb mentioned this pull request Nov 27, 2023
@idan idan added this to the 1.101 milestone May 9, 2024
@idan
Copy link
Contributor

idan commented May 9, 2024

Fixed in the batch of commits around 93c4bcd. Thank you!

@idan idan closed this May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants