-
Notifications
You must be signed in to change notification settings - Fork 243
Simplify dependency checking in bundle/install
#1606
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
base: master
Are you sure you want to change the base?
Conversation
This interface still doesn't make sense to me though - if we are listing dependencies as an argument to The "old" style of dependencies, where we reference more detail descriptions of where the dependencies come from should be able to work in parallel to the existing dependency mechanism, not replace it. The current dependency mechanism is mainly meant to prevent breakage of installed packages, and maintain some internal consistency. The old style of dependency management lives along side as Would it be possible for your package manager to generate the
Perhaps, but I don't think that the above changes are the solution. |
The existence check that
Yes, it is but this seems to me an extremely unsatisfactory solution. It would mean that my tool would create a ‘magic’ value in I just assumed the answer to this was yes but don’t we want users to stop using For what it’s worth, I agree that it is nicer if the user can just call |
It occurred to me that a compromise might be that :dependencies [{:name "spork" :git "https://github.com/janet-lang/spork"}] Would that be a better solution from your perspective? |
I like the last solution, where any non-string values would be ignored by the built-in |
Cool! I'll rework the PR. |
This PR simplifies the dependency checking that occurs in
bundle/install
. Rather than read the dependencies that may be present ininfo.jdn
, it relies on the caller passing the dependencies as an indexed data structure under the:dependencies
argument. It also clarifies some of the binding names.Background
Janet includes a
bundle/install
function that can be used to install code 'bundles' (i.e. packages).1bundle/install
checks for the presence of an 'infofile' that exists at either<bundle-root>/bundle/info.jdn
or<bundle-root>/info.jdn
and defines a struct/table. If this struct/table contains a:dependencies
key that maps to a tuple/array,bundle/install
checks that the dependencies listed in the tuple/array are installed. If they are not, installation of the bundle fails.The way that Janet checks that a dependency is installed is to check whether a directory exists in
<syspath>/bundle
with the value in tuple/array mapped to:dependencies
. This necessarily means that a user can only list dependencies using the name of the bundle (e.g."spork"
) and not an address to a bundle (e.g."https://github.com/janet-lang/spork"
).Rationale
I'm currently exploring making a Janet bundle manager and would like users to be able to define dependencies as they can in
project.janet
files. Currently, dependencies that are defined inproject.janet
can be:"spork"
)"https://github.com/janet-lang/spork"
){:repo "https://github.com/janet-lang/spork" :tag "eb8ba6bd042f6beb66cbf5194ac171cfda98424e"}
)I would like to see more adoption of
info.jdn
rather thanproject.janet
but I'm concerned this limitation will prevent people with dependencies in their project from using it because it will produce unexpected results.Implementation
This PR removes the dependency checks that are based on the contents of
info.jdn
. Instead,bundle/install
checks whether the dependencies passed under the argument:dependencies
are installed. It still stores the dependency bundles names in the bundle manifest under the:dependencies
key so that dependencies are not inadvertently uninstalled if they are in use by other bundles.Limitations
This is arguably a breaking change. As is visible from the changes to
test/suite-bundle.janet
, a caller needs to pass the dependencies tobundle/install
for it to work the way it does currently. I think this is acceptable for two reasons:I believe that the current implementation is itself broken insofar as a user would expect that
:dependencies
can take values that are the same as those taken by:dependencies
inproject.janet
. To the extent that this behaviour is being relied upon, it is the behaviour that is broken and that should be fixed.From my searches of GitHub, nobody (to a first approximation) is actually relying on this functionality directly. The
pm.janet
module in Spork does rely on this but it can be updated to pass["spork"]
as part of the call it makes tobundle/install
relatively easily.Footnotes
Added in 1.35.0 and released on 15 June 2024. ↩