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

Allow local directory global install with 'bpkg install -g' #170

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jwerle
Copy link
Member

@jwerle jwerle commented Dec 25, 2023

Fixes #169

This PR also normalizes command strings that may have a leading " or ending "

@thewebmasterp
Copy link

thewebmasterp commented Dec 26, 2023

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:

{
    "name": "script A",
    ...
    "dependencies": {
       "local_dependency_package_B": "master"
    }
}

So how would you specify local_dependency_package_B which would be a path in this case?

@jwerle
Copy link
Member Author

jwerle commented Dec 26, 2023

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:

{
    "name": "script A",
    ...
    "dependencies": {
       "local_dependency_package_B": "master"
    }
}

So how would you specify local_dependency_package_B which would be a path in this case?

We could address this by allowing the branch or version in a dependency to also include a file: protocol to indicate a local file path

@samlikins
Copy link
Member

samlikins commented Dec 26, 2023

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'
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 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
Copy link
Member

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()?

bpkg/lib/install/install.sh

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
Copy link
Member

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?

https://github.com/bpkg/bpkg/pull/170/files#diff-4ffd36319be99fb7f82206db54bcc36ed2fbc2eb5078a685ab4357eae032fd8dR192

cmd=${cmd#\"}
cmd=${cmd%\"}

Copy link
Member

@hyperupcall hyperupcall left a 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

@jwerle
Copy link
Member Author

jwerle commented Feb 3, 2024

I have been super busy I am sorry! I will get back to this very soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Can't install a package from a local directory
4 participants