search

shermp / go-fbink-v2

Go FBInk wrapper, Vers. 2
GNU Affero General Public License v3.0
13 stars 2 forks source link

Update to FBInk 1.13.0 #2

Closed NiLuJe closed 5 years ago

NiLuJe commented 5 years ago

Absolutely untested, I just made sure it compiled ;p.

Also haven't implemented the new dump/restore stuff, just updated everything else ;).

Mainly because I stupidly broke checkouts of older FBInk releases with stupid submodule shenanigans, and I needed it to build for Kobo-UNCaGED ;).

shermp commented 5 years ago

Looks good! Thanks. I would have gotten around to it eventually... Just one little nitpick, but otherwise no problem with this.

Kobo-UNCaGED should build with current master however, without needing this PR, but I'm not complaining!

shermp commented 5 years ago

Did a bit more of a look. I don't see any issues to prevent me merging this.

The FBInkDump struct as it is will probably change once I actually implement that feature, but it's doing no harm by existing, so will leave as-is for now.

NiLuJe commented 5 years ago

Yeah, I tried to keep the naming in spirit with what you did, but I kinda lost inspiration when FBInkDump's time came ;p.

NiLuJe commented 5 years ago

The issue was with brand new checkouts, when it tries to clone the FBInk submodule (well, technically, the submodules of the FBInk submodule ;p), it does so using the current gitmodules, which points to a repo where the requested commit doesn't exist anymore, because I force-pushed like an idiot ;).

If you had an existing checkout pre-my-own-fuckup, all would be well ;).

shermp commented 5 years ago

Thanks for doing this.

If I were to do it all again, I might do things a bit differently. The current wrapping method was conceived before there were so many structs to wrap...

(On a sidenote for your KU fork, if you've changed to your own local forks for go-fbink-v2 and/or KU, you will need to modify the import paths in the go files to match your local paths. Yes, it's a pain. Go finally has "proper" module support, but I haven't yet reconfigured KU for it, so still using the old GOPATH method)

NiLuJe commented 5 years ago

Ah, good to know ;). I wasn't necessarily planning on touching the Go bits, but good to know, as it's indeed the kind of thing that could have left me scratching my head for a while ;).