Skip to content

Conversation

behrica
Copy link
Contributor

@behrica behrica commented Jan 16, 2023

This solves #8
my use case is to run launchpad from inside Docker.

This requires to set bind to "0.0.0.0" and have a stable port, so I can specify the port to expose while starting Docker.

ctx)

(def before-steps [find-free-nrepl-port
(def before-steps [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an extra newline?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, some emacs automation. I can remove it

Copy link
Member

@alysbrooks alysbrooks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I think @plexus should review this change because I'm not sure what API he has in mind for the project as a whole—I don't want to approve this if he had other plans for -b and -p, for example.

Also, just to be clear to other reviewers, picking a port at random and choosing 127.0.0.1 were the existing behavior.

(assoc ctx :nrepl-port (or (get-in ctx [:options :nrepl-port])
(free-port))))

(defn get-nrepl-bind [ctx]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if this isn't technically correct, but I think of binding as applying to a port and address. I would use adddress instead of bind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100 % sure on this.
nrepl.cmd has three options related to "networking"

-b/--bind ADDR Bind address, by default "127.0.0.1".
-h/--host ADDR Host address to connect to when using --connect. Defaults to "127.0.0.1".
-p/--port PORT Start nREPL on PORT. Defaults to 0 (random port) if not specified.

I think "host" is something different... Only used when using the nrepl.cmd to connect to an other repl.

We could as well allow it to be configured as part of the "launchpad"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's following nREPL, then I think bind makes sense.

@behrica
Copy link
Contributor Author

behrica commented Jan 17, 2023

I want to add as well that nrepl itself can be configured via a "nrepl.edn" file, see here:
https://nrepl.org/nrepl/usage/server.html#server-configuration

This does not work current for launchpad, because it "overwrites" the port. So maybe this is an other way to solve the issue.

@behrica
Copy link
Contributor Author

behrica commented Jan 17, 2023

all three solutions are fine for my usecase (run launchpad from Docker):

  • new cli options
  • options in 'launchpad' file
  • make sure, that the configured ":bind" and ":port" in a user supplied nrepl.edn is not overwritten

@behrica
Copy link
Contributor Author

behrica commented Jan 17, 2023

I am now even thinking that option 3 is the best one.
It would allow to simplify the launchpad code, and remove the "find port" handling altogether.
nrepl default behavior is the same as programmed in lauchnpad (find a free port)

@alysbrooks
Copy link
Member

@behrica I like option 3, although being able to configure it both in nrepl.edn and the CLI might be ideal, actually.

@plexus plexus merged commit 6f4eae4 into lambdaisland:main Jan 20, 2023
@plexus
Copy link
Member

plexus commented Jan 20, 2023

Thanks @behrica, this looks good as a first step. I have a generic mechanism in mind for setting options in dev.local.edn, or ~/.config/deps.edn, with CLI args overriding dev.local.edn overriding ~/.clojure/deps.edn.

As for removing the find-port handling, we need to know which port was picked later on, that's why launchpad determines the port itself.

I'm not sure yet if we want to handle nrepl.edn, like I said we'll have our own config system, introducing another config file to check raises questions of which one takes precedence, and sets the expectation of parity with nrepl which we may not be able to offer. (do we support every setting in nrepl.edn? do we need to add code every time they add a setting? what if launchpad's interpretation is not exactly the same, etc.). It also means another location to check when troubleshooting.

@plexus
Copy link
Member

plexus commented Jan 20, 2023

Released in

[com.lambdaisland/launchpad "0.15.79-alpha"]
{com.lambdaisland/launchpad {:mvn/version "0.15.79-alpha"}}

plexus added a commit that referenced this pull request Jan 24, 2023
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.

3 participants