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

[Initialization] ZCompile all plugin *.zsh files and load init.sh plugins #260

Closed
wants to merge 4 commits into from

Conversation

YonatanAhituv
Copy link
Contributor

@YonatanAhituv YonatanAhituv commented Apr 5, 2018

Updated login_init.zsh to zcompile all patterns matching {Plugin-dir}/**/*.{zsh,sh} and {Plugin-dir}/*.{zsh,sh}.
Along with a minor change, to also load init.sh plugins.
This allows us to remove external plugin overrides and increase speed, along with greater compatibility with plugins like enhancd which use init.sh rather than init.zsh.

@YonatanAhituv YonatanAhituv changed the title Updated ZCompiling to compile all .zsh files [Initialization] Updated ZCompiling to compile all .zsh files Apr 5, 2018
@YonatanAhituv YonatanAhituv changed the title [Initialization] Updated ZCompiling to compile all .zsh files [Initialization] Updated login_init to zcompile all .zsh files Apr 5, 2018
@YonatanAhituv YonatanAhituv changed the title [Initialization] Updated login_init to zcompile all .zsh files [Initialization] ZCompile all plugin *.zsh files and load init.sh plugins Apr 5, 2018
@YonatanAhituv
Copy link
Contributor Author

YonatanAhituv commented Apr 6, 2018

@ericbn @Eriner
Can you look over this?

@YonatanAhituv
Copy link
Contributor Author

@ericbn @Eriner Ping

@@ -36,7 +36,7 @@ fi
if [[ ! -d ${zmodule_dir} ]]; then
print "No such module \"${zmodule}\"." >&2
else
for zmodule_file (${zmodule_dir}/init.zsh \
for zmodule_file (${zmodule_dir}/init.{zsh,sh} \
Copy link
Member

Choose a reason for hiding this comment

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

I think we should still only match init.zsh (not .sh) here.

for file (${zmodule_dir}/init.zsh \
${zmodule_dir}/{,zsh-}${zmodule}.{zsh,plugin.zsh,zsh-theme,sh}); do
for file (${zmodule_dir}/**/*.{zsh,sh,zsh-theme} \
${zmodule_dir}/*.{zsh,sh,zsh-theme}); do
Copy link
Member

Choose a reason for hiding this comment

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

If we want to be less specific here, I think the glob should be ${zmodule_dir}/(^*test*/)#*.{zsh,zsh-theme,sh}(.N) -- don't match *test* directories (see zsh-users/zsh-syntax-highlighting#443 and #205), only match plain files, and enable NULL_GLOB in this glob (no need to do setopt and unsetopt outside).

@Eriner, what do you think? This will potentially make login_init take more time zcompiling more files.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have an issue with the zcompiling taking longer - even if it takes 2m (spoiler alert: it won't) on first run, who cares? It's smart enough that it won't redundantly zcompile unecessarily, so that penalty will only happen once and as a result every subsequent shell will load faster. Sounds like a good trade-off to me.

Copy link
Member

@Eriner Eriner left a comment

Choose a reason for hiding this comment

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

@ericbn I made this review but never submitted it. Whoops!

for file (${zmodule_dir}/init.zsh \
${zmodule_dir}/{,zsh-}${zmodule}.{zsh,plugin.zsh,zsh-theme,sh}); do
for file (${zmodule_dir}/**/*.{zsh,sh,zsh-theme} \
${zmodule_dir}/*.{zsh,sh,zsh-theme}); do
Copy link
Member

Choose a reason for hiding this comment

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

Why are we zcompileing .sh files and .zsh-theme files?

Copy link
Member

Choose a reason for hiding this comment

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

Also, globbing to zcompile all modules isn't what we want. We only want to zcompile cyclically-complex and verbose files. The reason for this is that for small files, the time to interpret and execute the .zsh file is less than the read time of the (larger) .zwc, especially on spinning disks.

zcompile is only useful for cases when interpretation is the bottleneck, and when I/O on the larger pre-compiled file is marginally better than the straight interpretation.

Copy link
Member

Choose a reason for hiding this comment

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

Of course, we should not zcompile .sh files! 🤦‍♂️

What I like about this idea is that it makes zcompiling more general, and we would be also compiling extra files for whatever plugins are installed (not just extra files for syntax-highlighting or history-substring-search as we do further down). Maybe we can improve the glob by limiting by file size, like larger that 1K for example:

${zmodule_dir}/(^*test*/)#*.{zsh,zsh-theme}(.NLk+1)

(Also see my comments right below)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think a glob based on filesize is a good way to go about this. 👍

# zcompile all prompt setup scripts
for file in ${ZIM_HOME}/modules/prompt/functions/prompt_*_setup; do
zrecompile -pq ${file}
done

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm out of the loop here, but why is this removed?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

If we cover this w/ a glob based on filesize, LGTM.

@ericbn ericbn closed this in d74ceed Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants