kitsunyan / pakku

Pacman wrapper with AUR support
GNU General Public License v3.0
130 stars 8 forks source link

Does not build with Nim 1.2 #57

Closed JalilArfaoui closed 3 years ago

JalilArfaoui commented 4 years ago

With nim-1.2.0-1

GEN: completion/bash
GEN: completion/zsh
NIM: lib/tools
CC: stdlib_assertions.nim
CC: stdlib_io.nim
CC: stdlib_system.nim
CC: stdlib_parseutils.nim
CC: stdlib_strutils.nim
CC: stdlib_pathnorm.nim
CC: stdlib_posix.nim
CC: stdlib_times.nim
CC: stdlib_os.nim
CC: stdlib_strtabs.nim
CC: stdlib_streams.nim
CC: stdlib_osproc.nim
CC: bisect.nim
CC: stdlib_sequtils.nim
CC: install.nim
CC: tools.nim
NIM: src/pakku
src/pakku-0.14/src/utils.nim(5, 16) Warning: inherit from a more precise exception type like ValueError, IOError or OSError [InheritFromException]
src/pakku-0.14/src/utils.nim(8, 19) Warning: inherit from a more precise exception type like ValueError, IOError or OSError [InheritFromException]
src/utils.nim(214, 23) Warning: use `csize_t` instead; csize is deprecated [Deprecated]
src/pakku-0.14/src/args.nim(108, 15) Error: undeclared identifier: 'lc'
make: *** [Makefile:120 : src/pakku] Erreur 1
zqqw commented 4 years ago

I had noticed that too. The lc_useVersion branch in my fork builds as it adds the lc module as suggested by kitsunyan before, and uses the Nim useVersion compat build option. https://github.com/zqqw/pakku I have cleared some of the errors to build without that - swap csize for int, as it was an int and csize_t is a uint and caused other problems, swap object of Exception for object of CatchableError, and changed format.nim - (proc (a: string, c: float) {.closure.} = discard, proc {.closure.} = discard) (proc (a: string, c: float) {.closure, sideEffect.} = discard, proc {.closure, sideEffect.} = discard) Now it stops here: /pakku/src/feature/syncsource.nim(135, 25) Error: type mismatch: got <tuple of (proc (i0: int, i1: int){.closure.}, proc (){.closure.})> but expected 'tuple of (proc (a: int, b: int){.closure, noSideEffect, gcsafe, locks: 0.}, proc (){.closure, noSideEffect, gcsafe, locks: 0.})' make: *** [Makefile:120: src/pakku] Error 1 And I haven't got past that yet, that one seems a bit more difficult. It stops there with Nim 1.3 built recently from the Nim git repo as well.

ghost commented 3 years ago

@zqqw

Maybe it's related to this? https://github.com/nim-lang/Nim/issues/3127

zqqw commented 3 years ago

Building without the useVersion bit, I think it's something to do with scope - the update / terminate closures in proc printProgressFull* in src/format.nim don't want to work when called from proc cloneAndCopy in src/feature/syncsource.nim. That calls printProgressShare in src/format.nim which calls the first one mentioned above. If I disable the "not quiet" else so it doesn't happen it builds - but then there is another run time bug which I haven't looked at. I got rid of the terminate errors by declaring it as a top level exported proc pointer:

proc pointerterminate*() =
  echo()
type pptr = (() -> void)
var myp : pptr = pointerterminate

but to do that with update means declaring a bunch of other stuff as global variables as it uses some things earlier in the function before it enters the closure update proc so it would be quite messy.

zqqw commented 3 years ago

@C-Dev777 I tried what I think you meant, that is explicitly declaring missing types, but it didn't help, e.g.

#  proc update(current: int, total: int) {.closure.} =
  proc update(current: int, total: int): void {.closure.} =

Worth a try though, if that isn't what you were suggesting let me know. The new Nim 1.2.6 builds Pakku as well as 1.2.4 did, so no new breakage ahead for that version btw.

shirleyquirk commented 3 years ago

Swap the if statement around and it stops complaining!

  let (update, terminate) = if not quiet:
      printProgressShare(config.common.progressBar, tr"cloning repositories")
    else:
      (proc (a: int, b: int) {.sideEffect,closure.} = discard, proc () {.sideEffect,closure.} = discard)

i had to add sideEffect on all the update/terminate procs for a similar reason instead of using lc I went through and converted it all to use collect which isn't beautiful but is blessed by nim core.

In the course of which, this fix was added this is on current devel (1.3.5)

my PR: #58

zqqw commented 3 years ago

That's funny, I was reading that lc thread on the Nim forum earlier today as it seemed relevant to this, but it hadn't got to the part about the code being Pakku! I'll take a look and try it out, and hopefully merge it into my repo because at present that's where the AUR maintainers pointed pakku-git, and this is stable pakku in the AUR. (That will probably change back if kitsunyan was here more in future, but everyone has things to do I guess.) nim-legacy was introduced to fix the pakku build, so probably there's no need to worry about backward compatibility with old versions going forwards, as Arch tends to be about following the current version where possible. And people could still use an old pakku version to go with an old nim version if they wanted. kitsunyan likes lc, which is why I didn't continue replacing it with for loops: https://github.com/kitsunyan/pakku/commit/90d4c4be3bc15b2f594a0046c07b6f8654659ad5 But the lc module might itself stop working in future Nim versions, so perhaps adopting the new collect approach is better. Thank you for the help.

kitsunyan commented 3 years ago

I actually made some work earlier on this, though in fact I didn't update my arch linux system for about a year, so I didn't want to release it until I'll be able to test everything.

Pushed my fixes.