Closed ELLIOTTCABLE closed 8 years ago
Zgen already supports this, you can set ZGEN_AUTOLOAD_COMPINIT=false
if you don't want compinit to be called.
Compinit should only be called once, and it would make sense that zgen is the last thing that modifies fpath
and should be the one to call compinit. I just submitted a pull request to omzsh to add an option to prevent calling compinit. It would be better if zsh made it possible for zgen to "hide" compinit as a noop, but if that's possible I don't know how to do it :)
You're right that zgen should call compinit -i -C -d "${ZGEN_DIR}/zcompdump"
instead, since the whole point of zgen is loading a static configuration where our functions aren't changing.
You should submit a pull request with your lazy loading version, since sourcing zgen probably shouldn't do much of anything, and definitely shouldn't take more than 1 ms (currently everything before zgen-init on line 468 takes 20ms for me so I source the init file directly).
Er, I was talking about the call to compinit
inside -zgen-apply
, at the end of -zgen-save
… that doesn't seem to be governed by $ZGEN_AUTOLOAD_COMPINIT
? And what do you mean ‘hide it as a noop?’
As for doing it statically anyway: definitely a good idea; I think you're right, that lines right up with Zgen's purpose. (And aren't you guys already working on lazy-loading the whole of zgen itself in #46? Or is that something different? I'm happy to contribute my approach if it's not conflicting with existing work.)
Edit: Also, I'm pretty sure -i
is meaningless to compinit
in the case of -C
. -i
means the locations flagged as insecure are ignored (i.e. it means ‘show no warning’); whereas -C
means don't check the locations in the first place.
You're right about the flags.
I think it makes sense for zgen save to call compinit -d
, and then for all subsequent invocations of that init.zsh
file to call compinit -C -d
.
For hiding it as a no-op, I mean something like this
function compinit() { echo "Prevent plugins from invoking compinit." }
# ... run init.zsh
# ... maybe some personal stuff
# Make sure compinit happens once.
unfunction compinit
autoload -Uz compinit
compinit -C -d ...
Is there a better way to do that?
Oh, wow. Stomping on compinit
seems like a Really Bad Idea.
What's the rationale, here; prevent users from needing to know that Zgen is handling compinit for them? I think a much better approach is to not stomp on it, and make it very clear in the README that “Handling compinit for you” is one of Zgen's central features. (And, while you're at it, please don't call compinit yourself anymore, kay, thanks.)
As for the -d
and the -C -d
, oh, that just clicked. I see what you're trying to do: force compinit
to do the security-checks and compilations at the same time you're doing your own heavy work.
(One last note: why does Zgen, undocumentedly, currently move the zcompdump
into the Zgen dir? Why not just use the default ~/.zcompdump
?)
The rationale would be to prevent plugins, like oh my zshell, from calling compinit. The user would still be able to call it, given that zgen would restore it after it is done.
On Tue, Oct 20, 2015 at 8:07 PM, ELLIOTTCABLE notifications@github.com wrote:
Oh, wow. Stomping on compinit seems like a Really Bad Idea.
What's the rationale, here; prevent users from needing to know that Zgen is handling compinit for them? I think a much better approach is to not stomp on it, and make it very clear in the README that “Handling compinit for you” is one of Zgen's central features. (And, while you're at it, please don't call compinit yourself anymore, kay, thanks.)
— Reply to this email directly or view it on GitHub https://github.com/tarjoilija/zgen/issues/50#issuecomment-149740287.
So, I'm still totally confused as to how Zgen expects compinit
to be handled. I glanced through the source, but I just … don't quite get what the intent is, here. zgen
, on compilation, calls compinit
(as described above) … but init.zsh
doesn't call it in any way. Is this currently broken, then? Or is there some way it's intending to be used, that I'm not using it?
Meanwhile, if I understand it, your proposed change is to put compinit
inside init.zsh
(which would fix this very problem), and then put the compinit
-compilation in zgen save
?
Currently compinit
is called at the end of zgen.zsh, when it is sourced. Zgen save is the only thing that calls zgen apply, so nothing currently needs to change.
My suggest was primarily turning compinit into a no-op for plugins that zgen loads.
Beyond that, zgen.zsh should be entirely lazy loaded, and then compinit should either be a part of init.zsh or a function call in zgen.zsh
Yeah, that's what's going wrong over on my side, then; I'm lazy-loading zgen.zsh
itself (I mean, in my opinion, if it's working as intended, it shouldn't even need to be sourced, except when a zgen save
would be necessary.), so the compinit
at the bottom is never getting called.
Unless you or @m42e have objections, I think I'm going to dive into a slight re-write that allows for this intentional-lazy-loading; and then include the proposed dual-compinit
approach? Reading through #46, that seems desirable, yes?
Makes sense to me, the only reason I didn't already do this is because @tarjoilija never commented on the api we wanted to preserve. But so far anyone who has commented in issues does something to lazy load zgen anyways.
It would be good if you double checked that whatever autoloading doesn't take longer than the less than pretty way the top comment of #46 has it.
On Wed, Oct 21, 2015 at 7:54 PM, ELLIOTTCABLE notifications@github.com wrote:
Yeah, that's what's going wrong over on my side, then; I'm lazy-loading zgen.zsh itself (I mean, in my opinion, if it's working as intended, it shouldn't even need to be sourced, except when a zgen save would be necessary.), so the compinit at the bottom is never getting called.
Unless you or @m42e https://github.com/m42e have objections, I think I'm going to dive into a slight re-write that allows for this intentional-lazy-loading; and then include the proposed dual-compinit approach? Reading through #46 https://github.com/tarjoilija/zgen/issues/46, that seems desirable, yes?
— Reply to this email directly or view it on GitHub https://github.com/tarjoilija/zgen/issues/50#issuecomment-150056834.
Oh, I wasn't planning to write any lazy-loading logic; I was simply going to re-write to enable that precise pattern (i.e. make zgen.zsh
stop assuming that it's loaded at all; so that all you ever need to load is either zgen.zsh
or init.zsh
.) If somebody wants to then further extend my work with lazy-loading built into zgen.zsh
, that should be fine, too.
That sound like a good plan?
Yup, it would require anyone pulling zgen to also reset their init.zsh -- but I don't know why you would pull zgen without doing that.
On Wed, Oct 21, 2015 at 7:59 PM, ELLIOTTCABLE notifications@github.com wrote:
Oh, I wasn't planning to write any lazy-loading logic; I was simply going to re-write to enable that precise pattern (i.e. make zgen.zsh stop assuming that it's loaded at all; so that all you ever need to load is either zgen.zsh or init.zsh.) If somebody wants to then further extend my work with lazy-loading built into zgen.zsh, that should be fine, too.
That sound like a good plan?
— Reply to this email directly or view it on GitHub https://github.com/tarjoilija/zgen/issues/50#issuecomment-150057464.
:+1
also, if you two are on Freenode, hit me up. I'll chill in #zgen while I'm digging into this. <3
I've started on some work; I'd like some more eyes on it before I clean up the commit history and submit an actual pull-request:
zgen.zsh
.echo
with a printf
shim. That's pretty universally considered best-practice in my experience; but again: if at all unwelcome, just let me know!init.zsh
now makes sure the $ZGEN_RESET_ON_CHANGE
targets have changed at all (via modification-date) before bothering to SHA1 them.selfupdate
force a reset of init.zsh
(thx @mfarrugi)compinit
is now invoked differently by zgen apply
(on a generation-invocation) and init.zsh
(on a reuse-invocation). Not to mention that it actually respects $ZGEN_AUTOLOAD_COMPINIT
this time around, which it previously wasn't.
compdump
file; by default, it's left up to the default Zsh logic (so, $ZDOTDIR/.zcompdump
); this is a break with the current implementation, but only means a re-generation of zcompdump
by the end-user, and an old, now-ignored .zgen/zcompdump
file laying around. If a user does want it put somewhere specific, they can do so with $ZGEN_CUSTOM_COMPDUMP
.$ZGEN_AUTOLOAD_COMPINIT
and $ZGEN_CUSTOM_COMPDUMP
, as well as $ZGEN_COMPINIT_FLAGS
.I'll come back and take a second look at some of this in a few days, or whenever one of you has some time to comment on my changes; then I'll submit them! (=
I'm still not sure what the best way to handle the user or their plug-ins calling compinit is.. But other than that everything else looks (strictly?) better.
On Sun, Oct 25, 2015 at 10:39 PM, ELLIOTTCABLE notifications@github.com wrote:
I've started on some work; I'd like some more eyes on it before I clean up the commit history and submit an actual pull-request:
- There's some consistency refactoring mixed in there. If any of these are unwelcome, let me know, and I'll push a new branch with just the meaty changes.
- The codebase was objectively a mess in several ways (not like, “it's two spaces when it should be four,” more like, “it's two spaces some places, seven others, and one function just uses U+001F.”) I tried to suppress any opinions on my part, and just take whatever practice was most prevalent and unify it throughout
zgen.zsh
.- The one exception I made to the above was replacing
echo
with aprintf
shim. That's pretty universally considered best-practice in my experience; but again: if at all unwelcome, just let me know!- Status messages are now printed to standard-error, instead of standard-output.
- As discussed above, the generated
init.zsh
now makes sure the$ZGEN_RESET_ON_CHANGE
targets have changed at all (via modification-date) before bothering to SHA1 them.- For future changes, I made
selfupdate
force a reset ofinit.zsh
(thx @mfarrugi)- And of course,
compinit
is now invoked differently byzgen apply
(on a generation-invocation) andinit.zsh
(on a reuse-invocation). Not to mention that it actually respects$ZGEN_AUTOLOAD_COMPINIT
this time around, which it previously wasn't.
- Of not, I opted to not default to specify any particular location for the
compdump
file; by default, it's left up to the default Zsh logic (so,$ZDOTDIR/.zcompdump
); this is a break with the current implementation, but only means a re-generation ofzcompdump
by the end-user, and an old, now-ignored.zgen/zcompdump
file laying around. If a user does want it put somewhere specific, they can do so with$ZGEN_CUSTOM_COMPDUMP
.There's a couple more configuration bells and whistles, now: aforementioned
$ZGEN_AUTOLOAD_COMPINIT
and$ZGEN_CUSTOM_COMPDUMP
, as well as$ZGEN_COMPINIT_FLAGS
. I'll come back and take a second look at some of this in a few days, or whenever one of you has some time to comment on my changes; then I'll submit them! (=Reply to this email directly or view it on GitHub: https://github.com/tarjoilija/zgen/issues/50#issuecomment-151005946
Hi! I've been quite busy with the studies lately but I'll get back to this issue tomorrow.
@tarjoilija that's okay! I assumed you were busy; we've been working together so I can submit a clean, easy-to-quickly-review Pull Request for you. I'm not quite there yet!
I'll hilight you when I've got it put together. :P
Edit, for anyone glancing at this: I've gotten side-tracked with important life things, and haven't been able to dedicate the final burst of time I need to wrap this up. I'll get there eventually, I promise! :x
@ELLIOTTCABLE would you mind sharing your messy WIP state anywhere?
I'm not sure how I let this fall through the cracks!
I've cleaned up and submitted my work as #60. There's a couple of your comments that I wasn't able to address, though:
zgen.zsh
duplicating work that init.zsh
already does’ problem is out-of-scope for these changes; so I'm leaving that to future workgit pull
doesn't seem to return a useful exit-status I can work with; so I wasn't sure how to make zgen reset
only occur on pulls with changes. Unless somebody is zgen update
'ing on every shell invocation, I don't think this'll be a huge performance hit; left it for future work.compinit
invocations was fragile; unfortunately, I'm not familiar enough with the internals to come up with a better one? Suggestions welcome!Again, sorry I disappeared off the map! <3
So, in trying to speed up my shell launch-times, I discovered that one of the slowest parts of my boot is zgen invoking
zcompinit
without-u
or-C
or anything:(My own
zcompinit
takes nowhere near as long, as it's set-C
, and I manually clear out the~/.zcompdump
when I have a reason to.)At the moment, I've accidentally fixed this for myself by lazy-loading all of
zgen
itself, so thezcompinit
is never hit at load-time … but I can't figure out why zgen would be messing with this anyway. Can this be disabled somehow?