Vendor #1776

buhl · 2021-01-08T23:39:50Z

In #1587 we talked about a way to handle vendor libraries.

Description

This PR vendors https://github.com/brettlangdon/git-vendor to vendor libs into bash-it.
This PR also includes the preexec lib and fixes to the source where needed. It also
includes a new plugin that can ring the terminal bell echo -e \a when a long running
process finishes.

Motivation and Context

As discussed in #1587 I would be nice to have a better controlled/versioned way of
including libraries in bash-it. There have been talks of git subtrees and loader
scripts to aid the loading of plugins. This PR includes these tings.

How Has This Been Tested?

I have been running with the new preexec lib for over a year in a personal fork of bash-it
the git-vendor lib is only used be dev who wish to add or update libs. It wont have any
effect on regular users of bash-it

Types of changes

New feature (non-breaking change which adds functionality)
I feel I need to document how to add or update vendored libs. Where would be the best
place for this?

Checklist:

My code follows the code style of this project.
If my change requires a change to the documentation, I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
I have added tests to cover my changes, and all the new and existing tests pass.

NoahGorny · 2021-01-09T13:55:47Z

I am impressed by your efforts @buhl !
I agree that this needs to be documented somewhere. I guess the docs/development.rst can be a good location to include such information.

Also, if you add new plugins, please add then to clean_files.txt and lint them. I am unsure what to do about the vendored code, and we need to discuss whether we should lint it as well.

buhl · 2021-01-09T14:05:18Z

Thanks @NoahGorny!
I will be sure to tick that last boxes in the PR. I just wanted a go-ahead before putting to much work into it.
I don't think it's a good idea to lint the vendored code. It's libs we depend on, not source code we maintain.

NoahGorny · 2021-01-09T14:26:51Z

Thanks @NoahGorny!
I will be sure to tick that last boxes in the PR. I just wanted a go-ahead before putting to much work into it.
I don't think it's a good idea to lint the vendored code. It's libs we depend on, not source code we maintain.

I am also unsure whether we need to vendor git-vendor as well. Regular users do not need it, so we might as well just document how to install and use it for developers, without actually vendoring it inside Bash-it.

buhl · 2021-01-09T14:30:22Z

Thanks @NoahGorny!
I will be sure to tick that last boxes in the PR. I just wanted a go-ahead before putting to much work into it.
I don't think it's a good idea to lint the vendored code. It's libs we depend on, not source code we maintain.

I am also unsure whether we need to vendor git-vendor as well. Regular users do not need it, so we might as well just document how to install and use it for developers, without actually vendoring it inside Bash-it.

Sure, I guess that's a reasonable requirements for a developer.

cornfeedhobo · 2021-01-09T15:26:20Z

I really like where this discussion has gone. I notice that we're replacing the preexec library with a forked, and presumably better, version. Could we add erichs/composure to this PR as well?

buhl · 2021-01-09T16:26:21Z

@cornfeedhobo I don't see why we couldn't update composure as well once we landed on how to vendor libs.
But I think I would be more comfortable doing that in a separate PR (which I will gladly do once this is settled).

cornfeedhobo · 2021-01-09T16:36:11Z

@buhl My thinking was that since you're already doing two big changes, might as well as a third. In the opposite direction of breaking up significant changes, maybe we should update preexec in place, and then do vendoring then? Especially considering the diff between the changed preexec library and the updated composure one.

NoahGorny · 2021-01-09T16:48:51Z

I think we should remove the vendoring of git vendor, CR and merge this, and then continue with another vendoring PR of composure

buhl · 2021-01-09T16:50:58Z

@cornfeedhobo I am comfortable with the preexec changes for two reasons.

I have been using this since April and have had no issues.
Very little in the bash-it source code actually uses preexec.
Composure on the other hand is AFAIK used in all plugins, aliases and bash_it.sh so I feel that is a bigger change.
However I don't think the change will be a problem, I would just like to isolate it to it's own PR.

cornfeedhobo · 2021-01-09T16:57:25Z

@buhl those are good reasons. Thanks again for your contribution!

buhl · 2021-01-09T23:30:33Z

@NoahGorny @cornfeedhobo Sorry for the rebase. I had to make sure it could be done to give more clean PR's
Turns out it's a bit messy to de rebasing with subtrees. It should however, only be necessary when preparing a PR for a clean merge.
But I do think we/I need to play a bit more around with subtree before adding it to bash-it.
I have also updated the documentation regarding this issue. But it seems the tests for these are failing. I will look into this.
Other than that I removed git-vendor and added some files to clean_files.txt and checked that my changes also worked with the barbuk theme as it used preexec

buhl · 2021-01-09T23:39:29Z

I will try to make a test of the plugin I added as well.

buhl · 2021-01-10T12:37:49Z

I don't know why the lint is failing. Running the same script locally works.

cornfeedhobo · 2021-01-10T17:15:42Z

@buhl Hmm, barbuk should already be cleaned if you rebased off master correctly. Maybe rebase again?

The only other thing I can guess is that you've run into the same shfmt bug a few of us have. Essentially, our choice of formatting flags is causing a bug that adds whitespace on the first run in places you would not expect, like in bracket tests ([[]]). I've filed a bug upstream, but for now you'll just have to run shfmt like a dozen times on all the files you are adding.

buhl · 2021-01-11T11:12:08Z

Thanks @cornfeedhobo, It was an error on my part 🤦

buhl · 2021-01-11T14:35:24Z

I think maybe I should rebase this branch into fewer commits to make it more neat.
But @NoahGorny and/or @cornfeedhobo could I get some comments on the code and documentations?
I must admit that the trouble with rebasing a feature branch that is behind master makes me hesitate a little in recommending subtrees as the way to go for vendoring code.
I have also looked at https://github.com/ingydotnet/git-subrepo but AFAIK it doesn't support releases (as in tags) and it is overall a little more clumsy to use compared to git-vendor. However it creates a more clean git history 🤷

cornfeedhobo · 2021-01-11T14:40:10Z

@buhl no problem. I should have some free time in a few hours and will take a look.

nwinkler

Left a couple of minor comments...

docs/development.rst

plugins/available/cmd-returned-notify.plugin.bash

NoahGorny

Small nits from me, otherwise LGTM

test/plugins/xterm.plugin.bats

plugins/available/cmd-returned-notify.plugin.bash

docs/development.rst

NoahGorny · 2021-01-16T11:27:50Z

ping @buhl 😄

buhl · 2021-01-16T18:53:22Z

Sorry! My analog life called. I will try to fix the last few things and rebase the branch to make it merge ready tomorrow 😊

buhl · 2021-01-17T14:18:46Z

@NoahGorny I think it's about ready.
I did a rebase/force push to make the history a little nicer. So if you have pulled my branch be sure to delete it locally before pulling again.
I added a little more documentation. Other than that I think it's good to go?

NoahGorny

@buhl I left you a small comment, other than it LGTM

BTW, I see the git-vendor is not actively maintained, this is quite concerning, but if the tool works, we can use it and just fork it if needed

buhl · 2021-01-22T21:49:37Z

@NoahGorny Sorry, must have misser your comment. Will look at it tomorrow.

Regarding git vendor I referenced https://github.com/Tyrben/git-vendor because I looks like an actively maintained fork of the other repo. If the fork goes stale as well we can consider maintaining a fork our selfs.
But for now Tyrben is active in brettlangdon repos an seems to port pull requests to their own fork.

NoahGorny · 2021-01-22T21:56:16Z

@NoahGorny Sorry, must have misser your comment. Will look at it tomorrow.

Regarding git vendor I referenced https://github.com/Tyrben/git-vendor because I looks like an actively maintained fork of the other repo. If the fork goes stale as well we can consider maintaining a fork our selfs.
But for now Tyrben is active in brettlangdon repos an seems to port pull requests to their own fork.

Oh, I see, so ignore my comment, and instead add a note about the fact that we use the maintained fork and not the original repo (going to the supplied link might be confusing otherwise, I got quite confused)

Anyway, well done! I hope we can merge it soon and include all vendored code in this format

…it 7884535 git-subtree-dir: vendor/github.com/rcaloras/bash-preexec git-subtree-split: 7884535ed423ac27d3b6b473c61b1fe41905aca1

git-vendor-name: preexec git-vendor-dir: vendor/github.com/rcaloras/bash-preexec git-vendor-repository: https://github.com/rcaloras/bash-preexec git-vendor-ref: 0.4.1

Added a vendored lib loading routine in bash-it.sh Added documentation on how to vendor libs in bash-it Added and fixed plugins using preexec Added tests for two plugins Removed the old preexec lib

buhl · 2021-01-23T19:03:58Z

Hmm, themes/atomic/atomic.theme.bash and themes/base.theme.bash are failing the lint check. I did not touch these files but should I fix the errors in this PR?

buhl · 2021-01-23T19:09:10Z

@NoahGorny I added some text about the different git vendor repos. I think I have resolved all pending questions so now there is only the linting errors left mentioned in my previous comment.

NoahGorny · 2021-01-23T20:48:04Z

Hmm, themes/atomic/atomic.theme.bash and themes/base.theme.bash are failing the lint check. I did not touch these files but should I fix the errors in this PR?

Running locally, I saw shfmt complaining about the xterm plugin only, I added a fix commit, and I'll review this again now

NoahGorny

@buhl I made a couple of change, but now its LGTM

I'll let @nwinkler @davidpfarrell @cornfeedhobo voice their opinions as this is a big change. If we merge this, we should move all of our vendored code to be used in this pattern. I will help us control it!

buhl · 2021-01-23T21:40:41Z

Ah, I missed the linting errors in the xterm test because I saw errors in the two themes mentioned in an earlier comment.
Thanks for fixing them 😄

I will gladly help moving the other libraries to this vendor method if its accepted.

nwinkler · 2021-01-25T07:33:02Z

Looks good to me!

NoahGorny · 2021-01-27T20:52:53Z

Its about time we merge this, 🥳
@buhl you are welcome to move more things into the new vendor directory 😄

I added a lint check of the init.d directory, as it is our code

buhl · 2021-01-27T21:19:02Z

Sounds great 😄
I think I will have some time the upcoming days to vendor some libraries.
I have started a new issue #1818 to keep track of this process.
For now there is only only external library on the list

Also fix and lint the directory. This is our code, and as such, should be linted.

buhl mentioned this pull request Jan 8, 2021

Preexec as an updatedable library #1587

Closed

buhl force-pushed the vendor branch from caba62c to ba59e0d Compare January 9, 2021 23:22

nwinkler reviewed Jan 11, 2021

View reviewed changes

docs/development.rst Outdated Show resolved Hide resolved

plugins/available/cmd-returned-notify.plugin.bash Show resolved Hide resolved

nwinkler mentioned this pull request Jan 12, 2021

Cleaned some completion files and using _command_exists helper function #1782

Closed

8 tasks

NoahGorny reviewed Jan 13, 2021

View reviewed changes

test/plugins/xterm.plugin.bats Outdated Show resolved Hide resolved

plugins/available/cmd-returned-notify.plugin.bash Show resolved Hide resolved

docs/development.rst Show resolved Hide resolved

buhl force-pushed the vendor branch 5 times, most recently from 5ff2048 to 5809d92 Compare January 17, 2021 14:13

NoahGorny reviewed Jan 22, 2021

View reviewed changes

buhl force-pushed the vendor branch from 5809d92 to d81b239 Compare January 23, 2021 17:25

buhl added 2 commits January 23, 2021 18:28

Squashed 'vendor/github.com/rcaloras/bash-preexec/' content from comm…

c9a7837

…it 7884535 git-subtree-dir: vendor/github.com/rcaloras/bash-preexec git-subtree-split: 7884535ed423ac27d3b6b473c61b1fe41905aca1

Add "preexec" from "https://github.com/rcaloras/[email protected]"

5ad4979

git-vendor-name: preexec git-vendor-dir: vendor/github.com/rcaloras/bash-preexec git-vendor-repository: https://github.com/rcaloras/bash-preexec git-vendor-ref: 0.4.1

buhl force-pushed the vendor branch from d81b239 to 59afe9f Compare January 23, 2021 17:29

Adding preexec as a vendored library

29855ed

Added a vendored lib loading routine in bash-it.sh Added documentation on how to vendor libs in bash-it Added and fixed plugins using preexec Added tests for two plugins Removed the old preexec lib

buhl force-pushed the vendor branch from 59afe9f to 29855ed Compare January 23, 2021 17:31

plugins: xterm: Fix shfmt complaints

046e528

test: plugin: xterm: Use correct _command_exists invocation

6b37880

NoahGorny mentioned this pull request Jan 23, 2021

test: helper: cite plumbing metadata #1813

Merged

8 tasks

NoahGorny approved these changes Jan 23, 2021

View reviewed changes

Merge branch 'master' into vendor

9b04e11

NoahGorny enabled auto-merge January 27, 2021 20:53

buhl mentioned this pull request Jan 27, 2021

Moving external libraries into the vendor folder introduced in #1776 #1818

Open

19 tasks

vendor: Add init.d directory to clean_files.txt

c79d6cd

Also fix and lint the directory. This is our code, and as such, should be linted.

NoahGorny force-pushed the vendor branch from 1d57d26 to c79d6cd Compare January 27, 2021 21:21

NoahGorny merged commit 2388c74 into Bash-it:master Jan 27, 2021

buhl deleted the vendor branch January 27, 2021 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor #1776

Vendor #1776

buhl commented Jan 8, 2021 •

edited

Loading

NoahGorny commented Jan 9, 2021

buhl commented Jan 9, 2021

NoahGorny commented Jan 9, 2021

buhl commented Jan 9, 2021

cornfeedhobo commented Jan 9, 2021

buhl commented Jan 9, 2021

cornfeedhobo commented Jan 9, 2021 •

edited

Loading

NoahGorny commented Jan 9, 2021

buhl commented Jan 9, 2021

cornfeedhobo commented Jan 9, 2021

buhl commented Jan 9, 2021

buhl commented Jan 9, 2021

buhl commented Jan 10, 2021

cornfeedhobo commented Jan 10, 2021 •

edited

Loading

buhl commented Jan 11, 2021

buhl commented Jan 11, 2021

cornfeedhobo commented Jan 11, 2021 •

edited

Loading

nwinkler left a comment

NoahGorny left a comment

NoahGorny commented Jan 16, 2021

buhl commented Jan 16, 2021

buhl commented Jan 17, 2021 •

edited

Loading

NoahGorny left a comment

buhl commented Jan 22, 2021

NoahGorny commented Jan 22, 2021

buhl commented Jan 23, 2021

buhl commented Jan 23, 2021

NoahGorny commented Jan 23, 2021

NoahGorny left a comment

buhl commented Jan 23, 2021

nwinkler commented Jan 25, 2021

NoahGorny commented Jan 27, 2021

buhl commented Jan 27, 2021

Vendor #1776

Vendor #1776

Conversation

buhl commented Jan 8, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

NoahGorny commented Jan 9, 2021

buhl commented Jan 9, 2021

NoahGorny commented Jan 9, 2021

buhl commented Jan 9, 2021

cornfeedhobo commented Jan 9, 2021

buhl commented Jan 9, 2021

cornfeedhobo commented Jan 9, 2021 • edited Loading

NoahGorny commented Jan 9, 2021

buhl commented Jan 9, 2021

cornfeedhobo commented Jan 9, 2021

buhl commented Jan 9, 2021

buhl commented Jan 9, 2021

buhl commented Jan 10, 2021

cornfeedhobo commented Jan 10, 2021 • edited Loading

buhl commented Jan 11, 2021

buhl commented Jan 11, 2021

cornfeedhobo commented Jan 11, 2021 • edited Loading

nwinkler left a comment

Choose a reason for hiding this comment

NoahGorny left a comment

Choose a reason for hiding this comment

NoahGorny commented Jan 16, 2021

buhl commented Jan 16, 2021

buhl commented Jan 17, 2021 • edited Loading

NoahGorny left a comment

Choose a reason for hiding this comment

buhl commented Jan 22, 2021

NoahGorny commented Jan 22, 2021

buhl commented Jan 23, 2021

buhl commented Jan 23, 2021

NoahGorny commented Jan 23, 2021

NoahGorny left a comment

Choose a reason for hiding this comment

buhl commented Jan 23, 2021

nwinkler commented Jan 25, 2021

NoahGorny commented Jan 27, 2021

buhl commented Jan 27, 2021

buhl commented Jan 8, 2021 •

edited

Loading

cornfeedhobo commented Jan 9, 2021 •

edited

Loading

cornfeedhobo commented Jan 10, 2021 •

edited

Loading

cornfeedhobo commented Jan 11, 2021 •

edited

Loading

buhl commented Jan 17, 2021 •

edited

Loading