-
Notifications
You must be signed in to change notification settings - Fork 96
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
Allow local directory global install with 'bpkg install -g' #170
base: master
Are you sure you want to change the base?
Conversation
There's another related issue I stumbled upon while playing around with this fix: dependencies. Say that we have package A depending on B. Both A and B are packages residing in local directories on the system, rather than in remote repositories. So doing:
So how would you specify |
We could address this by allowing the branch or version in a dependency to also include a |
Firstly, this feature request isn't really related to this MR. This MR is for specifying a local project for the utility at the command line, while this feature request is for specifying in a project file. If this goes any further, turn this conversation into an issue, let's not start tacking feature requests on merge requests... So I think I'm missing something here in this thought process. A dependency in this situation has to do with defining the source location for installation of required projects of a specific project. You'd not only be installing a list of specified dependency projects, but specified versions of those projects. The way this is being discussed is more like defining how to link projects. That's more of a feature of repositories not projects. In other package management software, you'd update the managers listing of source repositories to include a new local repository. Repository management is a feature I was working at building out. The BPKG site listing and search is now setup to handle pointing to non-GitHub project repositories. My next feature to add to the utility was the revamp of source handling. This feature is just a natural consequence of that concept. Add a local repository override, that will reduce any resulting complexity. However, let's stop discussing this feature request on this MR. If we must continue this discussion, create a new issue and link this MR discussion for reference. Thank you, Sam |
json="$(cat package.json 2>/dev/null)" | ||
else | ||
popd >/dev/null || return 1 | ||
bpkg_warn 'Unable to determine bpkg.json' |
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 think this else
block could be removed, since this error case is covered by the [ -z "$json" ]
test below?
return 1 | ||
fi | ||
|
||
## install bin if needed |
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.
Should this section be extracted to a new function, along with its analog in bpkg_install_from_remote()
?
Lines 388 to 392 in c7f5a46
## install bin if needed | |
build="$(echo -n "$json" | bpkg-json -b -f='"install"')" | |
build=${build#\"} | |
build=${build%\"} | |
fi |
@@ -75,6 +75,14 @@ bpkg_runner () { | |||
local cmd="$1" | |||
shift | |||
|
|||
if [ "${cmd:0:1}" = "\"" ]; then |
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.
Could this code be replaced with something like the quote replacement that was added in a different part of this pr?
cmd=${cmd#\"}
cmd=${cmd%\"}
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.
Happy new years! Code looks good to me, I just had a few comments that could simplify a few things
I have been super busy I am sorry! I will get back to this very soon |
Fixes #169
This PR also normalizes command strings that may have a leading
"
or ending"