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

Removing Nim's knowledge of Nimble #654

Open
dom96 opened this issue May 11, 2019 · 26 comments
Open

Removing Nim's knowledge of Nimble #654

dom96 opened this issue May 11, 2019 · 26 comments

Comments

@dom96
Copy link
Collaborator

dom96 commented May 11, 2019

Context

Today the Nim compiler implements a feature called "nimble path" (--nimblePath). Where specifying --nimblePath:~/.nimble/pkgs/ expands to:

  • --path:~/.nimble/pkgs/jester-#head
  • --path:~/.nimble/pkgs/karax-v1.1
  • ...and so on for all latest versions that are inside the ~/.nimble/pkgs directory

This feature allows users to compile source code which imports one or more of these packages without having to create a Nimble file. In other words it makes coding in Nim more convenient.

Concerns

I've discussed this with Araq over the past few days, he would like to remove this code from Nim. The main arguments against this are:

  • Nim users are not aware of this feature and its implicit nature may mislead them
  • It's another piece of code that needs to be maintained in the compiler and synced with Nimble's behaviour

Other solutions

If this gets removed we need to replace it with a mechanism that is just as convenient but more clear to the user.

We've discussed two solutions.

Call Nimble from Nim to get path information

The idea would be that the Nim compiler executes a Nimble command that returns path information for the current compilation. Something like nimble getPaths:

dom@:~$ nimble getPaths
~/.nimble/pkgs/jester-#head
~/.nimble/pkgs/karax-v1.1

Essentially replicating the current logic in the compiler.

Transition period + use nimble c instead

Currently nimble c only works if it finds a .nimble file in the current working directory. We can change it to reuse the same logic as nim c. Alternatively we can hide this behaviour behind a new explicit flag, for example --latest. So instead of using nim c, you would use:

dom@:~$ nimble c --latest file.nim

The Nim compiler could give a nice error message when it detects that a module that is part of a Nimble package is imported, for example:

dom@:~$ cat file.nim
import jester
dom@:~$ nim c file.nim
Error: could not open 'jester'
Hint: This file is importing a module that was implicitly included from the --nimblePath. Use `nimble c --latest file.nim` instead.

After discussion, I believe we have both settled on the latter proposal.

@dom96 dom96 added the RFC label May 11, 2019
@stefantalpalaru
Copy link

The idea would be that the Nim compiler executes a Nimble command that returns path information for the current compilation.

Why would the Nim compiler depend on an external Nim package? It's bad packaging hygiene.

@zah
Copy link
Member

zah commented Sep 3, 2019

To make this proposal more concrete, is the following a correct understanding:

There is a new command-line option in the compiler specifying a tool that will be used to obtain package paths. For example, the switch can be named --pp: and it can accept an arbitrary command:

# Search for a local .nimble file, error out if one doesn't exist
nim --pp:"nimble paths"

# This lists all globally installed packages
nim --pp:"nimble paths --latest"

# List all package paths from particular local directories
nim --pp:"nimble paths ../third_party ./libs"

In order for this protocol to be future-proof, I think it needs to handle few more requirements:

  1. Diamond dependencies of the following form:
A -> B
A -> C
C -> D 1.0
B -> D 2.0

The package D must be resolved to a different path when imported from C or from B.

  1. Different packages with the same name appearing in your dependency tree.

If we start relying less on a central Nimble repository and more on http:// dependencies, package name collisions are quite possible. Again, we must be able to resolve the same import name to a different path depending on the context.

To handle these additional requirements, the list of package paths can consist of triplets with the following data:

<package-path> <dependency-name> <dependency-path>

<package-path> is the context in which a particular import appears. For example, the diamond dependency tree above will have the following paths:

package path dependency name dependency path
A_dir b B_dib
A_dir c C_dir
C_dir d D1_dir
B_dir d D2_dir

Cargo also allows you to rename a package dependency to solve the unlikely case of having collisions within the requirements of a single package. This can be handled in the future by specifying the rename within the Nimble file of the package where the collision happens.

@zah
Copy link
Member

zah commented Sep 3, 2019

If there is no option such as --pp in the compiler, the requirements above can be handled by allowing similar triplets to be passed with -p:. On Windows, Nimble may have to write out a temporary nim.cfg file on each invocation in order to deal with the command-line length limit there.

@yglukhov
Copy link
Member

yglukhov commented Sep 3, 2019

I like the "Call to nimble" approach. In fact, I advocated for it several times before. However, a plain list of paths is not extensible enough protocol, as much as I like its simplicity. I think it needs to be rather a config list, so that nimble getNimConfig would output something nim cfg compliant, so that in future we will be able to implement local package renames, as @zah suggests (I would also like local package optimization options, defines, etc, but no rush here :) ).

@stefantalpalaru

Why would the Nim compiler depend on an external Nim package? It's bad packaging hygiene.

Nim compiler is already aware of the nimble dir, nimble files and nimble packages. The suggestion is to make it aware only about nimble instead. Because, let's face it, it already is.

@zah

There is a new command-line option in the compiler specifying a tool that will be used to obtain package paths.

I don't see the point of doing this, but as long as the default is "call to nimble", and you're just arguing for a way to override it (in case of unlikely event), I'm fine with it.

@dom96
Copy link
Collaborator Author

dom96 commented Sep 3, 2019

Honestly, I still prefer the proposal that has been settled on (i.e. the last proposal I described above). That way Nim doesn't need to know anything about Nimble nor does it need to implement any complex mechanisms to call other processes which will give it paths (KISS really makes sense here).

@zah
Copy link
Member

zah commented Sep 4, 2019 via email

@yglukhov
Copy link
Member

yglukhov commented Sep 4, 2019

Honestly, I still prefer the proposal that has been settled on

Well that's a good proposal as well. In fact it has a potential advantage of treating nim itself as a dependency and calling the right version of nim.

My only concern is nim will become an almost unusable command on its own.

@Araq
Copy link
Member

Araq commented Sep 4, 2019

Nimble can generate a project specific nim.cfg file, no need for anything else IMO.

@zah
Copy link
Member

zah commented Sep 4, 2019

We need to agree on a concrete spec though, because @bobeff is waiting on us.

If Nim doesn't have any knowledge of Nimble, the primary problem in my opinion is that IDEs and text editors will have to treat Nimble as the canonical way to interact with Nim - Builds will have to be launched with nimble build and we'll need additional commands such as nimble suggest and nimble check to handle all other features of an IDE.

On a first glance, this is acceptable, but it introduces some complications for the "compiler as a service" use case. If the Nimble file of any project is modified to introduce a new dependency, any long-running Nimsuggest server has to be restarted to take into account the new paths. Admittedly, it can be argued that this is already the case because you can always introduce a new define in a config file that requires all of the incremental compilation knowledge to be discarded, so such monitoring is required anyway.

Also, please address my suggestions regarding the context-dependent imports. @yglukhov was the champion of this idea and I think we should get this right from the beginning. I can think of many occasions where Nim has resisted certain "complications" for years only to succumb to the real-world requirements eventually. In the end, it's always costlier to fix such things later than earlier.

@dom96
Copy link
Collaborator Author

dom96 commented Sep 4, 2019

Nimble can generate a project specific nim.cfg file, no need for anything else IMO.

@Araq this is just a small implementation detail, which tbh I disagree with, Nimble writing even more files would suck. But let's not argue about it now please.


@zah indeed, this proposal will mean that nimble will need to be used more often. I think this is the right approach (everything else sounds too convoluted, plus this is also how other package managers work, most notably cargo)

Diamond dependencies of the following form:

A -> B
A -> C
C -> D 1.0
B -> D 2.0

The package D must be resolved to a different path when imported from C or from B.

This is trivial for Nimble, but it requires Nim compiler support which I doubt is trivial to implement. As of right now you cannot pass --path:$NIMBLE_DIR/pkgs/D-1.0 and --path:$NIMBLE_DIR/pkgs/D-2.0 because then Nim won't know which D package to import (well, currently it'll just pick the first one, but that won't work when B needs 2.0).

Different packages with the same name appearing in your dependency tree.

This is again something that needs to be solved at the Nim-level. Nim is completely unaware of packages, so you cannot just "rename a package" like you can in Rust. We should be discussing this in the context of Nim, not Nimble, just like the above feature.

@Araq
Copy link
Member

Araq commented Sep 4, 2019

No, it's not "a small implementation detail", it's one solution to all the problems that @zah outlined.

@bluenote10
Copy link
Contributor

Nimble can generate a project specific nim.cfg file, no need for anything else IMO.

I really like that idea. From a clean code perspective it would be strange if the compiler (core layer) makes calls to Nimble (outer layer). Nimble's role is to compute search paths. If they can be passed via a nim.cfg it would lead to nice decoupling and avoids the misguided dependency.

Also, wrapping & duplicating all compiler CLIs with nimble seems a bit tedious.

@dom96 I wouldn't mind making the nim.cfg produced by Nimble explicit, but if it helps we could maybe hide it away in nimcache?

@dom96
Copy link
Collaborator Author

dom96 commented Sep 5, 2019

Maybe I don't understand what @Araq means. If so, can someone please correct what I'm missing?

Here is what I think @Araq wants:

nimble build -> nimble generates .nim.cfg file with --path entries -> nimble calls nim c file

This is what Nimble already does:

nimble build -> nimble calls nim c --path:... --path:... file.

@Araq's proposal does the same, but with an extra (and IMO minor step that's not worth arguing about for now). Am I completely misunderstanding Araq's proposal here? I see mention of the compiler calling Nimble, maybe it has something to do with that? (I personally don't think that's a wise approach).

@Araq
Copy link
Member

Araq commented Sep 6, 2019

It's nimble setup and then it creates or edits $project.nim.cfg with a section like

# begin: nimble generated section
--path: "foobar"
--path: "morehere"
# end: nimble generated section

and then I can use nim, nim check, nimsuggest, and they all understand the setup. Plus it's not hidden from me what Nimble does. Plus there is no need for Nim to know anything about Nimble. Plus Nimble does not need to grow more support for "pass this flag onto Nim already". (Though having this support would still be nice.) Plus I can use nim_temp (my debug Nim) easily to debug complex Nim programs that happened to use Nimble.

@zah
Copy link
Member

zah commented Sep 6, 2019

@Araq, the problem with this approach is that the paths will be user-specific (e.g. /home/andreas/.nimble). The project's nim.cfg file is intended for committing to git.

One way to solve this problem is to have a separate cfg file for nimble (or user-local settings in general), or to extend the config system to also search for a cfg file in the nimcache dir (that would be quite a bit more complicated).

@Araq
Copy link
Member

Araq commented Sep 6, 2019

The --paths can use a $nimbledir variable. Then Nim still needs to know about $nimbledir. Until we remove $nimbledir because it should die...

@dom96
Copy link
Collaborator Author

dom96 commented Sep 7, 2019

It's nimble setup and then it creates or edits $project.nim.cfg with a section like

The biggest problem with this idea IMO is that you'll need to run nimble setup every time you change your .nimble file, that will be a pain in the ass. This idea predicates that everyone uses Nim the same way you do: as a Nim language developer, which really is not the case and as the language scales will not be the case even more.

and then I can use nim, nim check, nimsuggest, and they all understand the setup.

This is indeed a problem, but I would prefer to solve it a different way. I've got some ideas but we should discuss them separately.

Plus it's not hidden from me what Nimble does.

If the experience becomes seamless then the user really won't need to care about what Nimble does.

Plus there is no need for Nim to know anything about Nimble.

Your solution doesn't solve this either, Nim only needs to know about Nimble to make implicit discovery of Nimble packages possible. If you don't care about that then that's fine, but it's an orthogonal point to what you're proposing.

Plus Nimble does not need to grow more support for "pass this flag onto Nim already".

What other support is needed? All flags are passed to Nim with nimble c already.

Plus I can use nim_temp (my debug Nim) easily to debug complex Nim programs that happened to use Nimble.

This is a niche case that you can solve yourself in many different ways.


The solution we have already settled on is intuitive and practically removes Nim's knowledge of Nimble. I'm getting really frustrated that even though we've already agreed to it you are now arguing for yet another random thing that adds more problems.

@genotrance
Copy link
Contributor

Let me start with the original argument:

Nim users are not aware of this feature and its implicit nature may mislead them

If this can be explained further, it will be helpful. I don't see what's the concern.

Besides, this functionality makes the compiler easy to use and avoids all the subsequent concerns raised in this issue. Nim depends on the environment setup by Nimble and cannot be ignorant of it without breaking the current convention so I disagree with the issue title in principle. How it is accomplished is another story.

It's another piece of code that needs to be maintained in the compiler and synced with Nimble's behaviour

I appreciate the concern but it is < 60 lines of code. And where is the algorithm really changing to warrant a discussion? Again, some more detail here will help understand this better.

Solution-wise, there are two options:

  • Nim/suggest will run Nimble and it will return paths in stdout or create a nim.cfg equivalent, whatever is preferred

    • Con is that we still need complexity in the compiler
      • Need to know whether Nimble is available
      • Will be slow spawning a process every run
      • Does Nimble really need to run everytime or can it be optimized
    • Having the user run nimble setup manually to use packages breaks the current user experience
  • Remove and use the algorithm directly from the Nimble repo

    • koch already knows how to pull the Nimble repo - simply do it as part of koch boot
    • koch boot can use --path:dist/nimble/src and import a Nimble module that provides nimblePath() functionality that gets compiled in
    • Csources can skip this functionality if required since you don't really need Nimble packages to bootstrap Nim
    • You get the separation and reuse while maintaining the user experience and performance

Overall, I don't see what's the value of this effort yet. There are solutions to do some cleanup but there is not yet any crucial reason for Nim to be ignorant of Nimble.

@Araq
Copy link
Member

Araq commented Sep 8, 2019

The solution we have already settled on is intuitive and practically removes Nim's knowledge of Nimble. I'm getting really frustrated that even though we've already agreed to it you are now arguing for yet another random thing that adds more problems.

Well it's related to how "Lockfiles" should work and I don't want nimsuggest to call nimble paths. Previously we ignored nimsuggest and related programs.

The biggest problem with this idea IMO is that you'll need to run nimble setup every time you change your .nimble file, that will be a pain in the ass.

Well, Nimble can be smart about it and update it automatically...

This idea predicates that everyone uses Nim the same way you do: as a Nim language developer, which really is not the case and as the language scales will not be the case even more.

That's an impertinence, I've much experience with Nimble and the workarounds people use to make it work with eg. cmake.

@dom96
Copy link
Collaborator Author

dom96 commented Sep 8, 2019

@genotrance thank you for asking these questions. I am completely with you with regards to your points. Frankly I simply got frustrated with the amount of people who keep raising this as a unimaginable sin that Nim is aware of Nimble, so I decided to write this proposal down finally so we can resolve it and so that I can stop hearing it. The concerns I list in the original post are those that I've gathered mostly by reading these criticisms and some of them come from @Araq.

Perhaps others can explain more about why they feel it as a horrible thing that Nim knows just that little bit about Nimble to make their life easier.

And where is the algorithm really changing to warrant a discussion?

To be fair the algorithm does change, rarely but it does. There is a PR to change it from @bobeff to introduce lock file support for example.

@dom96
Copy link
Collaborator Author

dom96 commented Sep 8, 2019

@Araq

Well it's related to how "Lockfiles" should work and I don't want nimsuggest to call nimble paths. Previously we ignored nimsuggest and related programs.

How do you come to this? If you extend the solution we agreed to to these programs then surely you should be thinking that nimsuggest et al will be called via Nimble, and then Nimble will simply resolve the paths and pass them onto nimsuggest?

To be honest, if we have a first-class Language Server, then I don't think it calling nimble paths would be the worst thing in the world (it could for example detect changes in the .nimble file and automatically update the paths).

Well, Nimble can be smart about it and update it automatically...

Yes, this could work, but please specify exactly what you mean. It's very difficult to discuss ideas when you just say "Nimble can be smart about it", what does that mean? I bet you that my guess is far different than yours. For example, I can see two ways, both with their problems (but I wouldn't be surprised if you're thinking of something completely different...):

  • Nimble runs as a daemon and automatically detects changes to .nimble files, then syncs them into the .nim.cfg file.

    • Problem: Running Nimble as a deamon changes its architecture significantly.
  • Nimble updates .nim.cfg files automatically when you run nimble c.

    • Problem: You have to remember to run nimble c to get the sync to happen, if you want to use nim instead of Nimble then that gets old very fast.
  • Problem for both: You've got unsaved changes in the .nim.cfg file, then you change the .nimble file and nimble overwrites it from under you. Very poor experience.

In general, if you're serious about this proposal then please specify it completely. Zahary has found issues with it too and I don't know which solution to them you prefer. Talk about every edge case so that we're not going backwards and forwards coming up with problems and waiting on you to give us a solution.

That's an impertinence, I've much experience with Nimble and the workarounds people use to make it work with eg. cmake.

Share these workarounds with us then. How are we meant to make an informed decision when you withhold these learnings from us?

@stefantalpalaru
Copy link

Perhaps others can explain more about why they feel it as a horrible thing that Nim knows just that little bit about Nimble to make their life easier.

Imagine GCC calling dpkg to get its include paths, or having hardcoded dpkg-specific paths inside it.

@dom96
Copy link
Collaborator Author

dom96 commented Sep 8, 2019

Imagine GCC calling dpkg to get its include paths, or having hardcoded dpkg-specific paths inside it.

Yes, the idea is to make Nim package-manager-agnostic. But even GCC has a concept of include paths, Nimble path in the compiler is just slightly more specific and the only reason it's called a "nimble path" is because Nimble came first. It would make sense as a generic package discovery mechanism for Nim.

@bluenote10
Copy link
Contributor

A build tool naturally has a dependency to a compiler. A compiler should not have a reverse dependency back to a build tool. Not only to avoid coupling the compiler to a specific build tool, but also to simplify the flow of information. That's true for all build systems and compilers I'm aware of.

@dom96
Copy link
Collaborator Author

dom96 commented Sep 8, 2019

It doesn't, and that is not what the proposal is either. In no circumstances do I want Nim to execute Nimble.

@genotrance
Copy link
Contributor

Imagine GCC calling dpkg to get its include paths, or having hardcoded dpkg-specific paths inside it.

A build tool naturally has a dependency to a compiler. A compiler should not have a reverse dependency back to a build tool.

Even the first line for rustc says this:

Most Rust programmers don't invoke rustc directly, but instead do it through Cargo.

There's no explanation for why this approach is brilliant and greatly improves the user experience. Today I can do nim c hello.nim and it simply works. I don't need any cmake, configure or .nimble if I want to use Nimble packages.

I tried using Rust and it was ridiculous that I couldn't do anything without learning Cargo. Same with many other languages where there's this whole ecosystem of random tools that hide away the compiler. Meanwhile in Go, the package manager and build system are first class citizens.

Yes, you can still use the 500 different build tools with --noNimblePath if you think Nimble isn't up to the job. You could even use cmake with Nimble like this guy did for Rust. But this idea that Nim should be a dumb tool abstracted away so that we get project hygiene is an Engineering bias and does nothing for the end user.

I have already proposed a way to reduce the code separation issue. If there are real-world reasons that justify this effort, please take the time to share.

If we want to do this regardless then go all the way and remove Nimble from the Nim repo - no more koch nimble, and no more testing important packages.

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

No branches or pull requests

7 participants