nim-lang / nimble

Package manager for the Nim programming language.
https://nim-lang.github.io/nimble/index.html
Other
1.25k stars 190 forks source link

v0.9.0 doesn't install files generated in `before install` #549

Open genotrance opened 6 years ago

genotrance commented 6 years ago

All nimgen wrappers download and generate artifacts as part of the before install step. This includes checking out git repos of upstream C/C++ code and the generated .nim files from c2nim. All these need to be installed to ~/.nimble for the package to work.

The new design to scan package contents in v0.9.0 is done before before install so none of these artifacts are included in the install, effectively breaking every single nimgen wrapper.

If I were to add an installDirs = @["libsvm"] to all the .nimble files (this directory does not exist in the repo before before install), nimble complains that directory does not exist and requires an interactive response to proceed.

If instead, I check in an empty libsvm sub-directory, nimble fails as below.

nimble install -y
Setting up Git repo
Checking out artifacts
Processing libsvm/svm.h
Generating libsvm/svm.nim
  Verifying dependencies for libsvm@0.1.2
      Info: Dependency on nimgen@>= 0.2.3 already satisfied
  Verifying dependencies for nimgen@0.4.0
      Info: Dependency on c2nim@>= 0.9.13 already satisfied
  Verifying dependencies for c2nim@#head
      Info: Dependency on compiler@>= 0.16.0 already satisfied
  Verifying dependencies for compiler@#head
      Info: Dependency on regex@>= 0.7.1 already satisfied
  Verifying dependencies for regex@0.7.3
      Info: Dependency on unicodedb@>= 0.5.1 & < 0.6 already satisfied
  Verifying dependencies for unicodedb@0.5.1
      Info: Dependency on unicodeplus@>= 0.3.0 & < 0.4 already satisfied
  Verifying dependencies for unicodeplus@0.3.2
      Info: Dependency on unicodedb@>= 0.4 & < 0.6 already satisfied
  Verifying dependencies for unicodedb@0.5.1
 Installing libsvm@0.1.2
nimble.nim(1106)         nimble
nimble.nim(1044)         doAction
nimble.nim(474)          install
nimble.nim(385)          installFromDir
packageinfo.nim(518)     iterInstallFiles
packageinfo.nim(486)     iterFilesInDir
packageinfo.nim(486)     iterFilesInDir
packageinfo.nim(486)     iterFilesInDir
packageinfo.nim(488)     iterFilesInDir
nimble.nim(389)          :anonymous
tools.nim(98)            copyFileD
os.nim(1204)             copyFileWithPermissions
os.nim(599)              copyFile
oserr.nim(66)            raiseOSError
Error: unhandled exception: Permission denied [OSError]

If I now think of @Araq suggestion to check in the generated files, I need to do it for every wrapper for every OS (win/lin/mac) and for every Nim version since c2nim will generate code that potentially isn't compatible with an older version of Nim since it uses the latest compiler code to render.

If I decide to take on that effort and proceed that way, I now have to ensure the right .nim files are imported for the right OS / Nim version combo. Right now, I'm unsure if that can be pulled off via a static nim file along with macros and when() statements. If I have to do anything before install, it won't work again.

The way it stands, scanning package contents before before install means you cannot have any dynamic packages, everything has to be static and that kind of defeats the purpose of before install in my mind.

genotrance commented 6 years ago

Some more comments here - even if I do cache the generated nim files, I still need to checkout the upstream code since it is compiled into the consuming application. If I do that in before install, nimble won't copy that over.

Right now, the only functional workaround is to do the following:-

after install:
  withDir(staticExec("nimble path libsvm")):
    exec "nimgen libsvm.cfg"

However, since nimble is picky about what goes into the package, I also need to add:

installFiles = @["libsvm.cfg"]

And now that I have downloaded and generated untracked files in the package that nimble itself did not copy over, nimble complains on uninstall.

Overall, I feel nimble is too opinionated on package structure and contents and really restricts what's possible. It is a struggle to comply with all the rules when all you really want to do is deliver a package and run a few commands.

genotrance commented 6 years ago

So I can workaround the uninstall complaint by adding the following:-

before uninstall:
  withDir(staticExec("nimble path libsvm")):
    rmDir("libsvm")

But note that I now have to make these changes across every single wrapper and hope other nimgen users keep up.

Please confirm that the above approach will be supported and not broken going forward. Making package structure changes in before/after hooks needs to be supported.

dom96 commented 6 years ago

I don't want to recommend this as an ideal way going forward, it's just a workaround and I don't want you to be telling users to create workarounds.

I think we need a different solution. The recent changes were introduced to make it easier for packagers, not harder, obviously this needs to be fixed as the effect seems to be opposite (for at least a certain subset of users). Sorry that this is broken in this version of Nimble :(

genotrance commented 6 years ago

So I'm okay moving to this workaround for now since it works on 0.8.10 as well as 0.9.0. However, if a better method comes up in the next release, I don't know how it could be used while still being backwards compatible.

I don't know if nimble supports different tasks or nimble files by version. Do we have something like when version == x?

dom96 commented 6 years ago

Have you tried doing installDirs.add(...) inside your before install after nimgen is invoked?

genotrance commented 6 years ago

Wow - good idea! It works - the generated files are copied correctly.

On the flip side, I realized that using after install fixes the other issue where the nimgen dependency itself doesn't get installed in time. This is because dependencies are installed after before install. Today, if you don't have nimgen installed and try to install a wrapper, you get:

> nimble install libsvm
Downloading https://github.com/genotrance/libsvm using git
sh: 1: nimgen: not found
stack trace: (most recent call last)
/tmp/nimble_21554/githubcom_genotrancelibsvm/libsvm.nimble(25) installBefore
/tmp/nimble_21554/githubcom_genotrancelibsvm/libsvm.nimble(22) setupTask
/home/gt/.choosenim/toolchains/nim-0.19.0/lib/system/nimscript.nim(237) exec
/home/gt/.choosenim/toolchains/nim-0.19.0/lib/system/nimscript.nim(237, 7) Error: unhandled exception: FAILED: nimgen libsvm.cfg

With the after install workaround, you get both the nimgen dependency installed before it is executed as well as compatibility with v0.8.10 and v0.9.0.

LMK what you think.

dom96 commented 6 years ago

Awesome. That sounds like the perfect way to handle this. I don't see any problems with this approach so I think we can close this issue :)

genotrance commented 6 years ago

Okay I spoke too soon - before uninstall works on 0.8.10 but doesn't work on 0.9.0.

dom96 commented 6 years ago

You shouldn't need that if you're using what I suggested

genotrance commented 6 years ago

Ya, that's why I was confused - your installDirs.add(..) suggestion works to install the files but if nimgen isn't installed, it still fails. That was my previous comment.

Using after install fixes BOTH installing files on 0.9.0 and installing nimgen before doing anything since now deps are handled before processing. However, you now end up with untracked files that don't get deleted on uninstall.

Basically before uninstall worked on 0.8.10 and is broken in 0.9.0.

h3rald commented 6 years ago

...Same here. I need to download extra files before compilation, so I tried what you guys discussed here and, on Nimble 0.9.0:

So, to sum up, it would be nice if:

a) Adding installDirs.add as a before install hook worked, i.e. that the directory containing downloaded files could be considered installable by Nimble (in this way files would get removed automatically on uninstall, which would be awesome).

or, if the above is not possible

b) Provide a before uninstall hook that works (I checked the code and it looks like you are not executing hooks with the uninstall command... is this intended).

Can this issue be re-opened? Would you accept a PR? for which solution?

dom96 commented 6 years ago

Both solutions should work, so I'll accept a PR for both or either. I would prefer you solve this with a) though.

h3rald commented 6 years ago

Errr yeah... right. I tried a) but got lost with the whole script execution. Essentially installDirs, installExt etc. are updated within the script, but the modifications aren't passed to the outer execution scope, so updating any of these in a before hook has no effect.

Is there any way to read these variables after the script execution and manually update the ones used by tasks? I tried accessing the PSym resulting from the execution but I couldn't get much out of it.

Instead, I provided a PR for b) 😄

genotrance commented 5 years ago

Update on this issue - I was able to work around the files not being installed by creating an empty directory in the nimble file. See here.

For some reason, not all the nimgen wrappers needed this so I'm quite confused but this method has fixed every other wrapper that was breaking. I'm on the fence to close this issue since it is a simple workaround. An alternative is to check-in an empty directory into git.

As for wanting to install nimgen before being used, I could work around that as well by testing if the executable ran or not. See here.

genotrance commented 5 years ago

This is still broken. Before install works fine with special versions but not version tags.

https://github.com/genotrance/nimarchive/issues/1

dom96 commented 5 years ago

I'm marking this as low-pri since there is a fairly simple workaround and this is a fairly niche use case. Also, you're involved in Nimble now so you can fix it if you really think it's worth the effort :)