nimble icon indicating copy to clipboard operation
nimble copied to clipboard

Command to add dependencies into .nimble file

Open wecurse opened this issue 5 years ago • 10 comments

Hi

Nim newbie here, I was expecting nimble to either automatically or via a flag, allow me to add dependencies to the <project>.nimble file. Is there a way?

Thanks

wecurse avatar Feb 12 '19 20:02 wecurse

There isn't. How do other package managers allow this?

dom96 avatar Feb 12 '19 22:02 dom96

I was thinking of something along how yarn does it:

add: this will automatically add it to the dependencies of the package.json

yarn add <package>

-D flag: this will automatically add it to the dev dependencies of the package.json

yarn add -D <package>

global: kindof what nimble install does

yarn global add <package>

It seems they are just parsing the json config file, reducing values and returning it.

wecurse avatar Feb 13 '19 01:02 wecurse

Nimble files are source code, not data files so it might not be worthwhile to implement Nim code parsing for this feature. Especially considering #635, this seems even more unlikely.

Perhaps we could add simple text manipulations to add or extend the requires call but it could be brittle.

genotrance avatar Apr 18 '19 17:04 genotrance

One possibility is to convert the nimble file into AST and then manipulate and write it out. Problem is that you end up losing comments and newlines. nimpretty can do this but requires the compiler code to be built with -d:nimpretty. If this were a runtime flag, would be possible to read nim files, edit them and then write them out without losing information.

cc @Araq to see if he's open to changing the compiler for this. Might be this way for performance reasons but would be cool to do modify files with standard macros.

genotrance avatar Jul 20 '20 03:07 genotrance

We don't have the technology to do that, performance has nothing to do with it. I also don't understand what's so controversial about a mechanism like

# BEGIN nimble maintained code
dep "a"
dep "b"
# END

It's a common pattern inside Linux distributions, it's simple, obvious and it works.

Araq avatar Jul 20 '20 07:07 Araq

The main problem with that approach is that we need to do this in a way that doesn't break the thousands of nimble packages out there. Just considering the requires stmt, you could have:

requires "abc"
requires "abc", "def"
requires("abc", "def")
"abc".requires
requires(
  "abc",
  "def"
)

Do we just support all that and who knows what else, or simply add a new line for every new package the user sets? Should we add the nimble maintained section at the bottom? Between the top definitions and tasks? I'd be annoyed if a the package manager messed up the formatting of my code because it can only guess the structure.

Next, the user will ask to remove packages, for bumping versions - change requires("winim >= 1.2.0") to requires("winim >= 1.3.0") with a command and the problem only gets worse. Without a parser, the code can only be manipulated primitively. Then we might as well tell users to hand-edit the file and keep the complexity out of nimble.

The fact that a nimble file cannot be treated as data makes tooling difficult. I also don't know if accessing and modifying the AST and writing it back is any easier.

genotrance avatar Jul 20 '20 19:07 genotrance

You can simply look for # BEGIN nimble maintained code, if it exists parse the contents. If it doesn't exist then create it and add the requires line. There is no need to support various syntaxes since you'll be guaranteed that this code was added by Nimble (and Nimble will only ever use one single syntax).

dom96 avatar Jul 20 '20 21:07 dom96

Okay, so here's the changes required to make this happen. Referencing Nimble added section header as #HDR for the rest of this comment - actual structure can be figured out later.

Behavior

  • Only required deps will be maintained within #HDR
  • Nimble init should add empty #HDR
  • Current deps are available when a pkg.nimble is loaded so that needs to be done before processing further
    • On pkg load, only include last instance of a dep - ideally #HDR is after legacy requires() calls
    • Warn user of duplicates and suggest removing duplicate requires outside #HDR
  • Check if nimble file already contains #HDR if deps changes requested
    • If section is not present, warn user and add #HDR at the end of file or before first task definition
  • Modify deps as specified by user - add/update/remove
  • Write to a temporary nimble file and run nimble dump
  • If dump deps matches expectation, write to original nimble file and exit
    • If legacy dep duplicate was overshadowed by #HDR, user will be warned later but not an error
  • Failure conditions
    • If legacy dep was after #HDR and overshadowed user command, deps won't match - suggest user hand-edit and remove legacy deps then rerun command
    • If command was to remove a legacy dep, deps won't match - suggest user manually remove legacy dep
    • If pkg fails to parse altogether, show failure and exit

Details on add/update/remove commands can be discussed separately - meanwhile, feel free to provide your feedback on the above.

genotrance avatar Jul 22 '20 03:07 genotrance

Nimble init should add empty #HDR

Nope. Please don't pollute nimble files with this unless the user actually uses nimble add <pkg> (or whatever the command will be).

Keep this as self-contained as possible. The normal dependency reading shouldn't care about these #HDR comments.

dom96 avatar Jul 22 '20 10:07 dom96

You can also insert a single include nimblegenerated to the .nimble file and then create a nimblegenerated.nim file that Nimble can edit in any way it likes. Maybe easier to implement.

Araq avatar Jul 23 '20 08:07 Araq