Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix: allow zinit to be run from non-interactive scripts #227
fix: allow zinit to be run from non-interactive scripts #227
Changes from all commits
ec4e460
856ea58
00467c8
562998c
47156d8
711916b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these tests verify this fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in, are these tests running interactively?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are running non-interactively, as all zunit tests. I'm not aware of any way to run them interactively (which probably is because of my zsh skill) :-(
Given that the bug (this PR should fix) is about making zinit able to run in (non-interactive) scripts, this is exactly what is getting fixed and tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did run with this fix for the last weeks and redid my setup multiple times, so I'm fairly confident it will not break interactive sessions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this works. I'd expect it to fail as AFAIK it tries to run
make install
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All three places in zinit which i found run
command make -C "$dir" ${(@s; ;)${make#\!\!}}
so no mention ofinstall
(as far as I understand this line)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI: this is the wording of an example in the readme: from my reading, it's in line with the code, i.e. if
install
is not the default make target, you have explicitly supply the target:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc you shouldn't need the move ice, but I forget why. It was something that seemed somewhat magical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit odd, if its a completion, you shouldn't need to source it as well via the
pick
ice ... right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, no idea: given that this shows up as a CI failure when the fix is applied and the test does not actually check for the existence of the completion, I would argue that it documents the status quo. Should I open a new bug report about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the output of a zinit call on current main with the old call in an interactive session, showing that no completion got installed:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #310 for this problem -> I also added an interactive session there which shows that you need the rename and the pick to link the completion into the right dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vladdoster Probably this changed line broke zdharma-continuum/zinit-annex-patch-dl#6
... and the similar changes to
ZINIT_EXTS
lines below...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: the cp hook wasn't run and therefore the second cp which tries to copy the compiled files fails now. Pattern with retval is the same as a few lines above in mv hook.