zqqw / pakku

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

I mean it this time #4

Closed shirleyquirk closed 4 years ago

shirleyquirk commented 4 years ago

Sorry, I'm still learning how to git

so here it is, works on 1.2.6, as well as current devel (1.3.5) removes dependency on lc entirely, no need for --useVersion or a deprecated library. Hopefully makes things clearer and maintainable as a result? At the very least, is blessed as part of the stdlib

Again, i didn't use your solution to the csize_t problem because I did it from scratch from the main repo, ints over ffi when its spec'd as a size_t just doesn't sit well with me, even if i know it's fine the curl callback seems to me like it should probably be (mem: UncheckedArray[char] ? maybe for another time.

sorry, what else, i changed a couple little things to shut up the warnings under the devel compiler, which was extra pedantic about whether procs were gcsafe and sideEffect.

zqqw commented 4 years ago

I did it like that as a csize_t and a csize aren't equivalent, one is signed and the other unsigned, while an int was the actual equivalent. The only possible difference would be for 32 bit vs 64 bit and I figured the non-core 32 bit projects would complain if it bothered them :) The other reason was that in the code some csize values are treated as an int or interact with functions that take an int, so it was an int by a different name being used as an int. But I'm open to all suggestions of course.

shirleyquirk commented 4 years ago

csize_t is equivalent to a size_t, so that matches what libcurl and setgroups expect. the equivalent of uint not int

i did a bit more digging: the instances of csize in the code are: in utils.h:setgroups which is an {.importc grp.h.} (from glibc) this is defined as int setgroups(size_t __n, const __gid_t * __groups) and used only once in util.h in dropPrivileges so yes, i cast a len to a csize_t, which is of course the same as just passing an int to setgroups. but feels better somehow?

in curl.h:curlWriteMemory is the callack function sent to libcurl, which expects: size_t write_callback(char *ptr, size_t size, size_t nmemb, void *userdata); this is only used internally by curl this could in fact overflow, on a 32bit system with >2GB ram, e.g. a raspberry pi 4. however unlikely, i feel more strongly about that one.

Edit: ok, curl won't pass more than CURL_MAX_HTTP_HEADER (100K) at a time, so it's safe. but csize_t is the 'right' type.

zqqw commented 4 years ago

Yes, it had been working with int with no bug reports so far, but it sounds like you have researched it. Although you use lc or collect with version specifiers, you have it still using lc for the current nim version, but have deleted the listcomp module that lets it work. That only reproduces this, which is still valid to use with Nim: https://github.com/nim-lang/graveyard/blob/master/lc/src/lc.nim but by including it as kitsunyan suggested, it removes the need to install the extra module in the build process. So it would need to remain until Nim 1.4 is released and gets to Arch - nim-git is quite a big build so users may prefer to use the regular nim binary package. Oh, maybe you do something different altogether there, I shall have to look at this some more.

shirleyquirk commented 4 years ago

yeah, the UncheckedArray thing is just aesthetics too but what do you think about:

 proc curlWriteMemory(mem: UncheckedArray[char], size: csize_t, nmemb: csize_t,
   userdata: ref CurlInstance): csize_t {.cdecl.} =

   let total = size * nmemb 
   for i in 0..<total:
     userData.data &= mem[i]
   total 

?

shirleyquirk commented 4 years ago

yes, I've only got the NimVersion specifiers for a couple of bits that don't work on 1.2, so works with stock arch Nim and will continue to work.

pakku-git won't compile on devel Nim, chokes on the most complex of those lc's (feature/syncinstall.nim:912)

latest commit takes out all references to lc entirely

I did this work without checking pakku-git, so duplicated a lot of your work, silly me. and pakku-git compiles so not critical, but it looks like this might be useful eventually.

zqqw commented 4 years ago

Tried it with nim 1.2.4, 1.2.6 and nim-git, it all seems to build and work fine. Thank you, I had been hoping to catch up with nim-git so I could look at some things besides the update issues. If you come up with anything more then I'll be happy to include it here. I'm just an enthusiastic Pakku user trying to keep it working, as it's a small enough codebase that there is some hope to do so, even though I'm a Nim noob, but fortunately Nim is pretty good to work with. And Pakku can do things other AUR helpers cannot! I've no idea what kitsunyan plans btw, and I don't want to step on anyones toes so will keep going in this repo for now unless / until other developments occur. Perhaps kitsunyan will return to maintaining Pakku, or some of the other contributors will get involved, idk.

shirleyquirk commented 4 years ago

Thank you right back! Glad to help, caught me at a new arch install and I just couldn't let yay be the aur helper when I've been getting more committed to Nim. I do hope kitsunyan gets back in the game or passes the torch, but I've invested the time now so for sure I'm available to help going forward.