Recursive deps

[kgann]

This is a WIP implementation of recursive dependencies

I'm using tree-seq to walk the dependencies so if this ends up getting merged, we may want to consider adding it to pixie.stdlib.

To implement this I had to refactor some of src/project.pxi so I could reuse some code in the dust.deps namespace - unfortunately it makes the PR a little noisy.

I'm looking for feedback to the approach before continuing. So far it appears to work fine.

I also extracted the (load-file "project.pxi") calls to defcmd - If the cmd var contains a truthy :no-project metadata, the loading is skipped - probably should have been a separate PR. I was just thinking of people writing their own defcmd's. I can remove this if need be.

Thanks for taking a look!

[heyLu]

Wow, thanks a lot for working on this! Are there specific parts you want feedback on?

Yes, tree-seq should migrate to pixie.stdlib.

Do you have an example project using this? (I'm not sure yet how to test the code in dust in general, maybe we'll just have to add an example project for that.)

[kgann]

I'll open a PR for tree-seq soon.

I'd like some feedback on :no-project - i.e. is this even worth it?

Also the use of tree-seq - any better ideas? Do you have any future plans for dependencies that would make this approach difficult to work with in the future?

I tested quickly by having download and extract-to work on my local filesystem where I created a few mock projects.

What do you think about making these fn's multimethods? We could have options to fetch dependencies from bitbucket, cloning repos instead of fetching archives etc...

Something like:

:dependencies [[kgann/foo "branch-name" :source :bitbucket :type :clone] ;; clones a bitbucket branch
               [kgann/bar "1.0.0"]] ;; defaults to github tar

If this sounds good, I'd like to get some input on the types of options we'd like to implement and how best to specify them.

[heyLu]

I think :no-project simplifies the code for new commands, so it's good. And it's easier to change the loading command, which we'll have to do for #4.

The use of tree-seq is also fine by me, nice and simple. :) I don't have plans for dependencies, so that's ok as well.

They should probably become multimethods, yes. I have thought about different providers before, just haven't needed them so far. I've just created a separate issue for them: #13.

In the future we should also use exec (which does not exist yet) instead of sh, because I think it's currently possible to create malicious packages that execute arbitrary shell commands. But that has been the case before, so it's just a note for the future.

[kgann]

@heyLu How should we test this? Do you want to create some example projects under pixie-lang? It may be easier to test with local projects once that is implemented.

[heyLu]

Yes, local projects seem to be the way to go. So, I'd say we test it manually for now.

To be honest, I haven't thought too much about testing yet. It will likely be a combination of a Makefile/scripts and pixie tests, but I need a bit of time to think about it. Ideas & suggestions welcome!

[kgann]

@heyLu I made a few updates:

• commands that require a project will fetch dependencies implicitly
  • this is needed to generate --load-path correctly
• dust load-path option now writes .load-path
  • commands like dust repl can output dependencies as they are downloading
    • previously the command would "hang" until downloads were finished and then start the repl
  • when writing .load-path, iterate over all dependencies and ensure their :source-paths are added
• removed tree-seq as that was merged into pixie.stdlib
• fetching dependencies alters dust.project/*project* and assoc's a seq of all dependency project maps (from their project.pxi files) into :dependencies

Let me know if you spot any issues!

[heyLu]

Is :path used anywhere?

[kgann]

yes, right here

[heyLu]

ah sorry, i missed that. :)

[heyLu]

• not sure about implicit fetching, many languages don't do that. (go, node, ruby, python do it explicitely; only leiningen seems to do it implicitely?)

  in any case, we'd still need an "update" command. let's discuss this in a separate pull-requests, i like to keep things separated.

  as an aside, why is this needed to generate the --load-path options correctly?

• i think it could just print out the options instead of writing them to .load-path? am i missing something here?

[kgann]

not sure about implicit fetching, many languages don't do that. (go, node, ruby, python do it explicitely; only leiningen seems to do it implicitely?

I'm not sure about other langs that do it implicitly - I'll try to explain why I think it's necessary (I may be wrong)

in any case, we'd still need an "update" command. let's discuss this in a separate pull-requests, i like to keep things separated.

Yes, you're correct.

as an aside, why is this needed to generate the --load-path options correctly?

If a project depends on a and a depends on b, we need the load path to contain src locations for a and b. Until we fetch a, we don't know it depends on b. Make sense? Let me know if I'm way off on this - I wont be offended :)

i think it could just print out the options instead of writing them to .load-path? am i missing something here?

The dust script captures the output of dust load-path option. When I added the implicit dependency fetching, I needed to grab the last line of the output as the previous lines were things like "Downloading foo". I have a commit where I added ... | tail -1 to grab just the load path line to pass on to pixie-vm

We could use bash to print all lines except the last but then you'd still see the terminal hanging and then finally spit out all the "Download..." messages before loading the repl.

To clarify this one, the goal was to show the dependency fetching as it happens while you're waiting for your repl.

[heyLu]

If a project depends on a and a depends on b, we need the load path to contain src locations for a and b. Until we fetch a, we don't know it depends on b. Make sense? Let me know if I'm way off on this - I wont be offended :)

Couldn't we just do this with an explicit command, like get-deps was doing before? Then we'd fetch all the dependencies to deps/ and set up the --load-path options to include all those directories.

So the workflow would be download dependencies once, do whatever (including editing stuff, running code, starting a repl) and only fetching again when adding new dependencies or wanting to update them.

I have a commit where I added ... | tail -1 to grab just the load path line to pass on to pixie-vm
That commit doesn't seem to be in the pull-request yet.

[kgann]

Here is the tail -1 commit - kgann@7745a5c#diff-63712a25d63eb8793112cc6dc2641526R19

It was removed once .load-path was implemented.

One positive of .load-path, IMO, is that you can easily inspect the load-path sent to pixie-vm. Imagine startup errors - you don't have a way to see the load-path as it's sent to pixie-vm only the friendly printed version from dust load-path unless you inspect the source and add the option parameter. Not the end of the world by any means and it could be added as part of documentation.

The only issue I see is that commands like load-path, deps and test also require that you have ran get-deps recently enough to have the latest. I don't think this is too much to ask of the developer and your workflow example would suffice.

The implicit fetching was to curb issues where someone forgot or thought they had already fetched dependencies - sometimes I start implementing too much - sorry about that. There is a guard to prevent needless fetching.

I'll revert back to manually requiring a get-deps run and printing the load path if you'd like - I'm not married to any of this :)

[heyLu]

Ah ok, I understand why you added .load-path now.

Yes, please revert back the implicit fetching for now. It's certainly something to discuss, but we should do it in a separate pull-request/issue.

Sorry about the back-and-forth, but thanks a lot for working on this!

[kgann]

No worries - I wanna make this a great tool so feedback and criticism welcome!

To be clear, revert to explicit dependency fetching and keep .load-path or would you also like to continue printing the load-path as well?

[heyLu]

Yes, keep .load-path, then it's explicit what's being loaded and it's also faster. :)

[kgann]

I remember another reason why I went with implicit deps.

Since we need the project maps from each dependency (to inspect their :source-paths entry), I needed to fire off dust.deps/get-deps for load-path in order to read them all - the guard prevented re-downloading.

Removing the implicit deps will take a little refactoring to make the other commands that used to inspect the projects dependencies (deps, load-path, describe) work as expected.

Perhaps this is a sign that mutating p/project when resolving dependencies is not the way to go.

I'll give this a shot this weekend. Let me know if you have some better ideas or if the above doesn't make sense.

TL;DR a lot of commands rely on dependencies and their read project maps - right now only get-deps reads the maps - it wasn't an issue since all commands that require a project tried to fetch dependencies and read the project maps.

[kgann]

Made the requested changes. The only issue I see is that dust deps and dust describe do not list sub-dependencies but only current project.pxi dependencies. Perhaps this is OK.

I made a test repo kgann/dust-test that depends on heyLu/hiccup.pxi with :source-paths ["src" "lib"]

and a local project.pxi:

(defproject local-repo "0.1.0"
  :description "Local repo"
  :dependencies [[kgann/dust-test "0.1.0"]])

$ ls -a
project.pxi

$ dust deps
kgann/dust-test 0.1.0

$ dust describe
local-repo@0.1.0
  Description: Local repo
  Dependencies:
    - kgann/dust-test@0.1.0

$ dust load-path
Please run `dust get-deps`

$ dust get-deps
Downloading kgann/dust-test
Downloading heyLu/hiccup.pxi
$ ls -a
.load-path     deps     project.pxi
$ dust get-deps
# no output

$ cat .load-path
--load-path deps/heyLu/hiccup.pxi/src --load-path deps/kgann/dust-test/src --load-path deps/kgann/dust-test/lib --load-path src

$ dust load-path
deps/heyLu/hiccup.pxi/src
deps/kgann/dust-test/src
deps/kgann/dust-test/lib
src

$ dust repl
Pixie 0.1 - Interactive REPL
(darwin_x86_64, clang)
:exit-repl or Ctrl-D to quit
----------------------------
user => @load-paths
["/Users/kgann/src/pixie" "." "deps/heyLu/hiccup.pxi/src" "deps/kgann/dust-test/src" "deps/kgann/dust-test/lib" "src"]

[heyLu]

I do have one final thing: I think dust get-deps should always re-fetch everything at least with the current state of things.

The reason is that we will always need a command that updates all the dependencies, including updating already fetched dependencies to a more recent version. As we currently don't do anything "fancy" with dependencies, dust get-deps is that command, at least for now.

[kgann]

Ok, that makes sense. See 373c0f8

[heyLu]

Thanks! I think this is ready to merge now. Do you want to fix/change something else before merging?

[mpenet]

minor nitpick: wouldn't it be nicer not to rely on a global ref (deps) and pass it as argument everywhere it's needed instead?
The same would apply for project.

[kgann]

It's trivial to remove dust.deps/*deps* - it was there from a previous iteration. @heyLu what do you think about removing it? It's not needed.

[kgann]

I went ahead and updated the README and removed dust.deps/*deps* here

I've got no more plans for this branch. Let me know if you'd like me to squash some of these commits.

[heyLu]

Ah no, I think I like having the history availlable. So I'll merge this now.

Again, thanks a lot for working on this (and sticking with it). ✨