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

install.cmd: support pyenv, fix potential %CD% bugs #152

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Tenrys
Copy link

@Tenrys Tenrys commented Apr 17, 2024

I was having a bunch of issues with the install script, and had to tweak the bat file a bit for my purposes, mainly because I was using pyenv for my Python versioning.

I had an issue where I had pip and ttx as .bat files in the environment, not .exe files, so they would make the script exit early after being called if not called with call.

Not entirely sure why I had to append %CD% where I did, though, some commands were ignoring the pushd command above.

I threw these changes rather hastily, so I'm putting them out here in order to discuss these sorts of issues and gather feedback, if there are any better solutions.

@13rac1
Copy link
Owner

13rac1 commented May 9, 2024

Thank you for this PR! I don't have any Windows systems to test this with nowadays. Can other Windows users test and confirm? Please use the Github review tool.

DEL "emjname.ttx"
call ttx -t "name" -o "emjname.ttx" %MS_EMOJI_FONT_PATH% || GOTO :ERROR
call ttx -o %FINAL_EMJ_FONT_PATH% -m %EMOJI_FONT_PATH% "%CD%\emjname.ttx" || GOTO :ERROR
DEL "%CD%\emjname.ttx"
Copy link
Owner

Choose a reason for hiding this comment

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

We need to know why or if the %CD is necessary, otherwise it should be removed

Copy link

@win98se win98se May 17, 2024

Choose a reason for hiding this comment

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

%cd% means “current directory”. @Tenrys said that some commands were ignoring the pushd, so they added the %cd%s accordingly.

They could probably explain more about how it didn't work in their case, and the “before and after” results of %cd% additions.

Copy link
Author

@Tenrys Tenrys May 22, 2024

Choose a reason for hiding this comment

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

Here are two screenshots to illustrate why I added usage of %CD%.

%CD% absent:

(Do note that ttx also fails, as can be seen by the first error message in the picture)

WindowsTerminal_Wgj0wx9C5K

%CD% present:

WindowsTerminal_XjnoRmKWCi

I don't know if the fact that I am calling install.cmd from PowerShell has any incidence on it, but it should make it more reliable in that case if it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants