Skip to content

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pyrmont
Copy link
Contributor

@pyrmont pyrmont commented Jul 3, 2025

This PR simplifies the dependency checking that occurs in bundle/install. Rather than read the dependencies that may be present in info.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).1 bundle/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 in project.janet can be:

  1. 'short names' that match a value in a package listing (e.g. "spork")
  2. URLs to git repositories (e.g. "https://github.com/janet-lang/spork")
  3. a struct/table that defines a more complex location (e.g. {:repo "https://github.com/janet-lang/spork" :tag "eb8ba6bd042f6beb66cbf5194ac171cfda98424e"})

I would like to see more adoption of info.jdn rather than project.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 to bundle/install for it to work the way it does currently. I think this is acceptable for two reasons:

  1. 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 in project.janet. To the extent that this behaviour is being relied upon, it is the behaviour that is broken and that should be fixed.

  2. 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 to bundle/install relatively easily.

Footnotes

  1. Added in 1.35.0 and released on 15 June 2024.

@bakpakin
Copy link
Member

bakpakin commented Jul 4, 2025

This interface still doesn't make sense to me though - if we are listing dependencies as an argument to bundle/install, we could just check for their existence manually.

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 :jpm-dependencies.

Would it be possible for your package manager to generate the :dependencies and pass that to info.jdn somehow, in the same way that project-janet-shim does?

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 in project.janet. To the extent that this behaviour is being relied upon, it is the behaviour that is broken and that should be fixed.

Perhaps, but I don't think that the above changes are the solution.

@pyrmont
Copy link
Contributor Author

pyrmont commented Jul 4, 2025

This interface still doesn't make sense to me though - if we are listing dependencies as an argument to bundle/install, we could just check for their existence manually.

The existence check that bundle/install performs at the moment is a check that the value in the :dependencies tuple/array matches a directory in <syspath>/bundle. If we want Janet to support :dependencies having more complex ‘coordinates’ that refer to remote locations, you cannot do it this way unless bundle/install can resolve a coordinate to its bundle name. That means bundle/install needs to be able to fetch files from the remote location so that it can look at the bundle’s info.jdn or project.janet file to determine its bundle name. That then requires bundle/install to be able to use some combination of curl, git and tar. I had thought this was unacceptable as the bundle functions are only meant to know what is on the local file system. Am I wrong on this?

Would it be possible for your package manager to generate the :dependencies and pass that to info.jdn somehow, in the same way that project-janet-shim does?

Yes, it is but this seems to me an extremely unsatisfactory solution. It would mean that my tool would create a ‘magic’ value in info.jdn (mapped to :dependencies) that the user must be aware that they should not edit by hand (even though they wrote info.jdn) lest they cause it to get out of sync with the value mapped to, say, :ok-to-edit-dependencies that they are able to edit by hand and which can contain more complex coordinates.

I just assumed the answer to this was yes but don’t we want users to stop using project.janet and move to info.jdn? Isn’t it inherently fragile to have tools edit these files with information that the user is not meant to touch even though they are editing the rest of the file?

For what it’s worth, I agree that it is nicer if the user can just call bundle/install with a path to somewhere that contains info.jdn or bundle/info.jdn but that means either: (1) you can’t have complex coordinates in :dependencies (this seems like a huge regression compared to project.janet) or (2) bundle/install needs to be able to resolve bundle names from remote locations (which I think introduces a great deal of additional complexity to boot.janet). Given that, requiring callers of bundle/install to specify :dependencies seems the preferable interface to me.

@pyrmont
Copy link
Contributor Author

pyrmont commented Jul 5, 2025

It occurred to me that a compromise might be that bundle/install could support a coordinate that includes the bundle name and a remote location. This would mean you couldn't just have a URL but I personally would find that acceptable. Something like this:

:dependencies [{:name "spork" :git "https://github.com/janet-lang/spork"}]

Would that be a better solution from your perspective?

@bakpakin
Copy link
Member

bakpakin commented Jul 5, 2025

I like the last solution, where any non-string values would be ignored by the built-in bundle/install, or we would extract just the :name key, but the extra information could still be processed by other tools. However, that also begs the question of why not just add your own custom array of external dependencies to info.jdn, say :git-deps @[{...}], or :custom-package-manager-deps @[...]. However, such a compromise would be ok to me.

@pyrmont pyrmont marked this pull request as draft July 5, 2025 01:31
@pyrmont
Copy link
Contributor Author

pyrmont commented Jul 5, 2025

Cool! I'll rework the PR.

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

Successfully merging this pull request may close these issues.

2 participants