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

Frictionless Nimble directory structures #653

Open
Araq opened this issue May 9, 2019 · 12 comments
Open

Frictionless Nimble directory structures #653

Araq opened this issue May 9, 2019 · 12 comments
Labels

Comments

@Araq
Copy link
Member

Araq commented May 9, 2019

Currently Nimble is quite picky about the valid project structures, see https://github.com/nim-lang/nimble#project-structure. This is often annoying for Nimble users and even if it weren't, it's something people have to learn and remember and is hardly justified. "Namespacing" is often used as the justification but that is not necessarily Nimble's job. If package A and package B both offer utils.nim in their top level directories, it could still be possible to disambiguate manually like import A / utils. And soon enough the package's users will demand sane names anyway.

A slightly different, but more pressing problem is that Nimble only copies the .nim files from srcDir over to the "installation" destination. In other words, the installation environment differs from the devel environment. That is asking for trouble and even if it works, it means the users of a package do not see the example programs or the documentation because it is not "installed" at all!

Nimble should simply git clone a package followed by a git checkout to select the required version/commit. This would also simplify the creation of eventual pull requests that the package's user might want to do. But even if Nimble continues to download/copy the package's files without the hidden .git directory, it should not touch the directory structure and it should not "install" only a selected set of files.

So what previously was $nimbleDir/p-1.0.0/p.nim becomes $nimbleDir/p-1.0.0/p/src/p.nim, for example. For backwards compat the Nim compiler can then resolve an import like import p to import p/src/p.

Advantages

  • cd $nimbleDir/foo && nimble docs or any other build target works out of the box without any special logic in the project.nimble file.
  • Nimble's source code becomes simpler, fewer project structure rules to enforce.
  • Nimble becomes easier to use.
  • It seems to be completely backwards compatible.

Disadvantages

The Nim compiler must do a bit more work to resolve imports. However, already it needs to be aware of the $nimbleDir, versions and "nimble develop" redirections. It is not a big deal to add slightly more logic on top of this mess if Nimble becomes easier to use and understand as a result.

Open Questions

How to handle git repos that contain multiple nimble packages?

@Araq Araq added the RFC label May 9, 2019
@treeform

This comment has been minimized.

@treeform

This comment has been minimized.

@treeform

This comment has been minimized.

@Araq

This comment has been minimized.

@genotrance
Copy link
Contributor

To elaborate a bit more on the requirements:

  • On install, Nimble will copy package contents to $nimbleDir/pkgs/package-x.y.z/package instead of $nimbleDir/pkgs/package-x.y.z. This will ensure every package gets added to a unique namespace. Nimble should continue to call Nim the same as today: nim c --path:$nimbleDir/pkgs/package-x.y.z. The net effect is that a package that used to be import package will now need to be imported as import package/package.

  • Nimble will no longer scan package contents - does this mean skipDirs, skipFiles, skipExt, installDirs, installFiles and installExt should be deprecated or are we saying Nimble still allows package maintainer to decide what should and should not be installed? Retaining this functionality means some scanning of packages but not having an opinion on structure.

  • $srcDir today is location where .nim files are and by default, it is the root dir. This is why Nimble today allows importing nim files from the root. In order for src to be treated as the root, srcDir needs to be explicitly set. Does this RFC mean srcDir is set to src by default? Then there's still an opinion on package structure and it breaks every package that has nim files in the root or somewhere else. E.g. import nimterop/cimport because $nimbleDir/pkgs/nimterop-x.y.z/nimterop/cimport.nim and there's no $srcDir defined.

  • If it instead means $srcDir is no longer of interest to Nimble, backwards compatibility will be more challenging since now Nim needs to know where to look instead and will need to scan the .nimble file to redirect import package to import package/$srcDir/package.

Regarding using git clone, it works for regular packages but not for repos that have multiple packages. It is possible to do a sparse git clone but then package structure will be a bit different and might need some extra work to fix the namespace or backwards compatibility. I think this is best left to a separate RFC.

Additional advantages

  • Nimble can be enhanced to build docs by default and make it easy to open docs
  • Nimble can allow running tests on installed packages to verify they still work with local compiler

Another item worth discussion is whether the Nim redirection to resolve imports should be temporary and deprecreted eventually.

@dom96
Copy link
Collaborator

dom96 commented Sep 21, 2019

So I've hidden some comments that have gone off-topic, please create separate RFCs for those.

You might be surprised, but I do actually love this idea. There are still some concerns that I have though:

Semantics of the new Nim compiler package resolution

For backwards compat the Nim compiler can then resolve an import like import p to import p/src/p.

@Araq, you say this is for backwards compatibility, this implies that you do not want future code to use this shortcut. Are you expecting people to write import p/src/p in new code?

I also want us to be very specific about the semantics of this, can you come up with an implementation? Even in pseudo-code would be helpful to make sure we're on the same page.

Nimble's srcDir

As you may know, and @genotrance alluded to, the defaults for Nimble aren't that the srcDir is set to "src". We would need to change this as well. I'm happy with doing this but just so we're clear this will be a breaking change.

Semantics of how packages are installed that this RFC proposes

Just to be clear, let me specify concretely how Nimble will install packages if this RFC is implemented:

  • nimble install jester

Package structure will look like this:

.~/.nimble/pkgs/jester-1.0.0/jester/
├── changelog.markdown
├── src
│   ├── jester.nim
│   ├── jester
│   │   ├── patterns.nim
│   │   ├── private
│   │   │   ├── errorpages.nim
│   │   │   └── utils.nim
│   │   └── request.nim
├── jester.nimble
├── license.txt
├── optimisation.md
├── profile_results.txt
├── readme.markdown

Here is how I envision imports to work:

  • import jester -> ~/.nimble/pkgs/jester-1.0.0/jester/src/jester.nim
  • import jester/patterns -> ~/.nimble/pkgs/jester-1.0.0/jester/src/jester/patterns.nim

Note that I left some crap that I have in my repo (optimisation.md, profile_results.txt), there was a lot more of this. If you want Nimble to install all of this then you'll get a lot of generated files and binaries installed to your .nimble directory. IMO that shouldn't happen but that can be a discussion for another time.

Multiple packages in a single repo

How to handle git repos that contain multiple nimble packages?

This is actually simple. Nimble already gets a Git URL which includes the path to the package, all you have to do is effectively cd git-repo/pkgname && nimble install (let me know if that doesn't make sense).

@dom96
Copy link
Collaborator

dom96 commented Sep 21, 2019

I've just been going through the issues and wondered: while we're adopting this RFC could we also adopt scoped org packages? i.e. nimble install dom96/jester. Even if we don't adopt this now, it would be useful to think about for the future, I'm not sure how we would support this in the Nim compiler. @Araq any thoughts? (Here is one issue that this feature might alleviate: #574)

@genotrance
Copy link
Contributor

Re: using git clone

In order for this to be useful, we will need to add functionality in Nimble to convert a regular repo clone into a fork when the user wants to contribute to an installed package. It is easy enough to fork manually and nimble develop though.

Using git for updating a package isn't useful either since in theory, you want multiple versions of a package to be retained. Another complication is how nimble install from a directory will retain the git directory structure.

I vote for leaving git out of this RFC and dealing with that separately if even warranted.

Re: opinionated package structure

I'm not sure what the opinionated part is. Using srcDir is optional. The other defaults like using private or tests are simple enough.

I suggest leaving srcDir behavior as is.

I agree the entire package should be copied as is though. If anything, this seems like the only part of this RFC that is really warranted. The code changes required would be to resolve imports relative to srcDir if declared. This will require parsing the .nimble file and works with the ideas in #654.

Re: package namespacing

If we want to make changes to Nimble's package structure and resolution ($nimbleDir/pkgs/package-x.y.z/package instead of $nimbleDir/pkgs/package-x.y.z), we need to make sure it coexists with the ideas in #654 which wants to remove Nim's knowledge of Nimble.

While I understand the benefit of two packages not conflicting, I don't know how important this is and whether it needs to be mixed into the same RFC.

Re: multi-package repos

effectively cd git-repo/pkgname && nimble install

If we want the git repo retained in ~/.nimble/pkgs, this won't work since nimble install doesn't do that. Meanwhile, I've already suggested against retaining git.

@Araq
Copy link
Member Author

Araq commented Sep 23, 2019

If you want Nimble to install all of this then you'll get a lot of generated files and binaries installed to your .nimble directory. IMO that shouldn't happen but that can be a discussion for another time.

If it's harmless enough for "git clone", it's harmless enough for Nimble. Simple.

@dom96
Copy link
Collaborator

dom96 commented Sep 23, 2019

If it's harmless enough for "git clone", it's harmless enough for Nimble. Simple.

You might very well install local Nimble packages, and then you'll get all sorts of generated files. A solution might be to just parse .gitignore files.

@timotheecour
Copy link
Member

timotheecour commented Jan 11, 2021

@dom96 IMO nim-lang/RFCs#291 gives the missing ingredient to allow this RFC.

Under this RFC, you'll have:

  • no scope pollution, which is a real problem
  • no need for srcDir
  • no ambiguity
  • more flexibility as to how paths are resolved, when the need arises
  • can be done without a breaking change

example

constantine package defines helpers/static_for.nim at top level (https://github.com/mratsim/constantine/blob/master/helpers/static_for.nim) however, it violates package namespacing (import helpers/static_for or import pkg/helpers/static_for) should not compile for 2 reasons:

  • it may clash with some other module's helper's directory
  • it doesn't convey that it belongs to constantine package

instead, with nim-lang/RFCs#291 you'll have:
nimble passes --path:constantine:constantineClonePath/constantine (instead of --path:constantineClonePath/src as done currently)
and then these would work:

import constantine/primitives
import pkg/constantine/primitives
import pkg/constantine/../helpers/static_for # the `..` is resolved after pkg/constantine is resolved

links

@dom96
Copy link
Collaborator

dom96 commented Jan 11, 2021

@timotheecour I love it. Let's do it :)

Edit: Actually, thinking about it some more. This will break packages like jester which allow you to just import jester, you'll need import jester/jester, or am I missing something?

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

No branches or pull requests

5 participants